Skip to content

Conversation

sharady
Copy link
Collaborator

@sharady sharady commented Aug 24, 2016

From xcp-networkd.conf file we can configure enic-workaround-until-version="x.x.x.x"
to the version till enic driver workaround will be applied or
set the version to an empty string for not applying the workaround.

Signed-off-by: Sharad Yadav sharad.yadav@citrix.com

String.strip String.isspace (Unixext.string_of_file ("/sys/bus/pci/drivers/" ^ driver ^ "/module/version"))
with _ ->
warn "Failed to obtain driver version from sysfs";
""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an option type would be better.

@lindig
Copy link
Contributor

lindig commented Aug 24, 2016

I don't have a strong opinion about this but would use a different name for the flag: enic_workaround_enabled=true/false. It is shorter and shares with the other flag the same enic prefix.

@robhoes
Copy link
Member

robhoes commented Aug 24, 2016

The enic_driver_version config option should have a name that more clearly indicates what it is for, e.g. enic-workaround-until-version. We can also combine the two options into one, and say that the workaround is disabled if enic-workaround-until-version is the empty string.

try
let version_of vs = List.map int_of_string (String.split '.' vs) in
let first_non_zero lst = List.fold_left (fun a b -> if (a<>0) then a else b) 0 lst in
(first_non_zero (List.map2 compare (version_of vs1) (version_of vs2)) <= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not do the right thing if the lists are not the same size.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function is a bit hard to follow, and it took me a while to figure out how it is meant to work. Also, it is not obvious what the return value of the function actually means. Also, it looks like version_of is really more like a list_of_version...

How about something like this:

(* Returns `true` is vs1 is older than vs2 *)
let is_older_version vs1 vs2=
  let list_of_version vs = List.map int_of_string (String.split '.' vs) in
  let rec loop vs1' vs2' =
    match vs1', vs2' with
    | [], _ | _, [] -> false
    | a :: _, b :: _ when a < b -> true
    | _ :: tl1, _ :: tl2 -> loop tl1 tl2
  in
  loop (list_of_version vs1) (list_of_version vs2)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code looks more relaxed for comparing driver version in case version string having different count of digits. Thanks @robhoes I have updated the PR as per review comments.

From xcp-networkd.conf file we can configure enic-workaround-until-version="x.x.x.x"
to the version till enic driver workaround will be applied or
set the version to an empty string for not applying the workaround.

Signed-off-by: Sharad Yadav <sharad.yadav@citrix.com>
@robhoes robhoes merged commit 9f2af52 into xapi-project:master Aug 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants