CP-47754: Do not report errors attempting to read PCI vendor:product

[freddy77]

Different network card (like virtual ones or USB ones) do not have a vendor:product causing networkd daemon to report a lot of errors like

Error in read one line of file: /sys/class/net/eth0/device/device, exception Unix.Unix_error(Unix.ENOENT, "open", "/sys/class/net/eth0/device/device")
Raised by primitive operation at Xapi_stdext_unix__Unixext.with_file in file "lib/xapi-stdext-unix/unixext.ml", line 68, characters 11-40
Called from Xapi_stdext_unix__Unixext.buffer_of_file in file "lib/xapi-stdext-unix/unixext.ml" (inlined), line 155, characters 31-83
Called from Xapi_stdext_unix__Unixext.string_of_file in file "lib/xapi-stdext-unix/unixext.ml", line 157, characters 47-73
Called from Network_utils.Sysfs.read_one_line in file "ocaml/networkd/lib/network_utils.ml", line 156, characters 6-33

Do not report these missing file as errors.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e17089c) 49.07% compared to head (221e86e) 49.07%.
Report is 11 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5451   +/-   ##
=======================================
  Coverage   49.07%   49.07%           
=======================================
  Files          18       18           
  Lines        2319     2319           
=======================================
  Hits         1138     1138           
  Misses       1181     1181           

Flag	Coverage Δ
python2.7	53.38% <ø> (ø)
python3.11	55.85% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[robhoes]

This copies what read_one_line does, without logging on error. Another option is to add an optional argument ?(log = true) to read_one_line to control whether to log or not, so we don't need to duplicate the function.

[freddy77]

Indeed I'm a bit puzzled about that. I understand the change proposed. On the other side this read_one_line function is doing something "unexpected" with EINVAL error. I understand it's useful for some virtual files (like reading speed or carrier which can be read only when card is up). Potentially also that log option (OT: maybe ignore_errors should be a better name?) could be reused somewhere else but it makes the read_one_line more complicated. In different paths in the same file something different like Unixext.string_of_file path |> String.trim is used.

I was also wondering if we should only ignore some type of errors, not all. For instance EPERM or ENFILE although unlikely should be reported.

One other comment on this function is that it should return either ("<VENDOR>", "<PRODUCT>") or ("", ""), not something like ("<VENDOR>", "").

My knowledge about Ocaml tends to zero.

[edwintorok]

To ensure that last property you can move where 'try/with' is, if you move it one level up to the 'get_pci_ids' function and on error have it return "", "", it should ensure the consistency property you're looking for.

[lindig]

In general, I think the (existing) code is too complicated. Reading a few words from /sys could be simpler:

• https://github.com/xapi-project/xen-api/blob/master/ocaml/xenopsd/pvs/pvs_proxy_setup.ml#L102-L105
• https://github.com/xapi-project/xen-api/blob/master/ocaml/xenopsd/pvs/pvs_proxy_setup.ml#L259-L269
• Using scanf can do all the extra splitting and whatever you might need

[GabrielBuica]

I think let l = path |> Unixext.string_of_file |> String.trim is a bit easier to read.

[lindig]

If we have expectations towards what to find in /sys, we should not use explicit string operations to split and trim strings but I advocate using scanf.

[freddy77]

@lindig I have no idea how to implement your proposal and it looks like an additional change not having to do a lot with the problem this change is trying to fix. That code is assuming hexadecimal format like 0xHHHH but then it takes the HHHH verbatim so with scanf I suppose we would like to get the decimal number and then we'll need to format in hexadecimal again.

[lindig]

Compare the apparatus currently employed against this:

let finally f always = Fun.protect ~finally:always f

let scanning path f =
  let io = Scanf.Scanning.open_in path in
  finally (fun () -> f io) (fun () -> Scanf.Scanning.close_in io)

scanning "/sys/class/net/wlp0s20f3/device/vendor" @@ fun io ->  Scanf.bscanf io {|%i|} (fun d -> Printf.sprintf "%x" d)

- : string = "8086"

lindig@dell:xs$ cat /sys/class/net/wlp0s20f3/device/vendor 
0x8086

This is what I mean when I say the current code is too complicated. My code reads the hex value as an integer and emits it again as hex but without a prefix. I understand that this is not the problem you are solving but we should try to make things simpler.

[freddy77]

@lindig your proposed change is out of my reach, I understand roughly what is doing but giving my Ocaml knowledge is more complicated. Also your snippet does not address the intention of this pull request (handling missing files) but changes the file parsing using scanf+printf. However I thing you are also introducing a regressions; you are stripping leading hexadecimal digits while we want to retain them.

Can you propose a full solution addressing the device issue and the suggested scanf changes ?

[lindig]

Let's merge this. I will take a look at the code independently.

[lindig]

This is related to the interface renaming problem we have seen with iptables(8) stumbling over bridge names that exceed 15 characters.

[robhoes]

Looks fine, but we'll not merge it immediately.