Re: [PATCH v1 1/1] usertools/devbind: update coding style

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 12/2/2024 6:01 PM, Stephen Hemminger wrote:
> On Mon,  2 Dec 2024 15:09:34 +0000
> Anatoly Burakov <anatoly.burakov@intel.com> wrote:
> 
>> +    # For kernels < 3.15 when binding devices to a generic driver (i.e. one that doesn't have a PCI
>> +    # ID table) using new_id, some devices that are not bound to any other driver could be bound
>> +    # even if no one has asked them to. hence, we check the list of drivers again, and see if some
>> +    # of the previously-unbound devices were erroneously bound.
>> +    if not devbind.use_driver_override:
> 
> Why is tool still supporting out of date and no longer supported kernel?

The aim was 100% compatibility with the old script, but I agree these 
parts can be taken out as this kernel is no longer supported. This will 
definitely make the binding code simpler.

> 
> 
> 
>> +        choices=[
>> +            "baseband",
>> +            "compress",
>> +            "crypto",
>> +            "dma",
>> +            "event",
>> +            "mempool",
>> +            "misc",
>> +            "net",
>> +            "regex",
>> +            "ml",
>> +            "all",
>> +        ],
> 
> Would prefer that all the types are in table/list and the help just
> references that list. The next time a type is added, only one place
> needs to change.

It's a bit difficult to have *everything* as one list, as there are 
multiple places where we use this:

1) initial declarations at the top of the file (which I treat as "ground 
truth" for what sort of devices devbind aims to recognize)
2) categorization rules (which are inside Devbind class)
3) command line arguments
4) printouts

I suppose I can merge 3 and 4, but I don't see a neat way to specify 1) 
and 2) in a way that we can reuse elsewhere. I'll think on this though, 
thanks for the suggestion.

> 
> Also, I would not trust the output format of ip route not to change.
> If the utility has to parse output of ip command, use json (-j) instead.
> 
> This whole section of code is quite fragile:
> 
>>      if devices_type == network_devices:
>>          # check what is the interface if any for an ssh connection if
>>          # any to this host, so we can mark it later.
>>          ssh_if = []
>>          route = subprocess.check_output(["ip", "-o", "route"])
>>          # filter out all lines for 169.254 routes
>>          route = "\n".join(filter(lambda ln: not ln.startswith("169.254"),
>>                                   route.decode().splitlines()))
>>          rt_info = route.split()
>>          for i in range(len(rt_info) - 1):
>>              if rt_info[i] == "dev":
>>                  ssh_if.append(rt_info[i + 1])

The quoted code is from old devbind code, but I agree that relying on -o 
output is not ideal, and using -j will be better. I'll fix it in v2.

Thanks for your feedback!


-- 
Thanks,
Anatoly