openwisp/openwrt-openwisp-monitoring

[change] ModemManager device option needs to update to ctl_device

mips171 opened this issue · 6 comments

Hey,

Just letting you know, we are changing this from device to ctl_device upstream in the OpenWrt ModemManager protocol handler because of compatibility issues with DSA. I will link the PR here when it is in.

local modem = uci_cursor.get('network', interface['interface'], 'device')

How's this look? It looks like the uci_cursor.get returns nil if that entry doesn't exist, right?

        local compat_device = uci_cursor.get('network', interface['interface'], 'device')
        local device = uci_cursor.get('network', interface['interface'], 'ctl_device')
        local modem = (compat_device or device)

        if (compat_device and device) then
            modem = device
        end

        local info = {}

        local general = io.popen('mmcli --output-json -m '..modem):read("*a")

@nickberry17 yes, it should, I haven't tested the following modified version yet but if it works is a bit shorter but still readable:

local device = uci_cursor.get('network', interface['interface'], 'ctl_device') or \
               uci_cursor.get('network', interface['interface'], 'device')
local info = {}
local general = io.popen('mmcli --output-json -m '..device):read("*a")

The second lineand the or won't be executed in the future and can be removed at some point in the future.

Is it an inclusive or? Do we need to check if both are set, so we don't get device set to /sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1/sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1 In a sysupgrade scenario where settings are kept, both device and ctl_device could be set.

Is it an inclusive or? Do we need to check if both are set, so we don't get device set to /sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1/sys/devices/platform/soc/8af8800.usb3/8a00000.dwc3/xhci-hcd.0.auto/usb2/2-1 In a sysupgrade scenario where settings are kept, both device and ctl_device could be set.

The one with precedence should come first, the or is executed only if the first value is nil.

@nickberry17 yes, it should, I haven't tested the following modified version yet but if it works is a bit shorter but still readable:

local device = uci_cursor.get('network', interface['interface'], 'ctl_device') or \
               uci_cursor.get('network', interface['interface'], 'device')
local info = {}
local general = io.popen('mmcli --output-json -m '..device):read("*a")

The second lineand the or won't be executed in the future and can be removed at some point in the future.

Didn't like the \

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:326: unexpected symbol near '\'

Putting them on the same line:

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:330: attempt to concatenate global 'modem' (a nil value)
stack traceback:
	/usr/sbin/netjson-monitoring:330: in function 'specialized_info'
	/usr/sbin/netjson-monitoring:392: in function 'get_interface_info'
	/usr/sbin/netjson-monitoring:578: in main chunk
	[C]: ?

The longhand way works though

Putting them on the same line:

uci: Entry not found
lua: /usr/sbin/netjson-monitoring:330: attempt to concatenate global 'modem' (a nil value)
stack traceback:
	/usr/sbin/netjson-monitoring:330: in function 'specialized_info'
	/usr/sbin/netjson-monitoring:392: in function 'get_interface_info'
	/usr/sbin/netjson-monitoring:578: in main chunk
	[C]: ?

What's the modem here? I think it is renamed to device in other prior instances expect one giving error