CA-201728: Add new function is_vlan_in_use
#86
Conversation
| error "Error: could not read %s." vlan_dir; | ||
| [] | ||
| in | ||
| if List.mem vlan vlans then true else false |
There was a problem hiding this comment.
It's enough to say List.mem vlan vlans - the result is the boolean you are looking for
There was a problem hiding this comment.
Indeed!! Updated that line, Thanks :)
|
Lets wait for @robhoes review too, He might have some more thoughts for network backend switch ovs and linux bridge. |
|
We cannot do this check based on device names. We need to use sysfs to check whether the VLAN exists in the kernel. |
| let vlan_device = device ^ "." ^ (Int64.to_string vlan) in | ||
| let interfaces = | ||
| try | ||
| List.map (fun interface -> List.hd (String.split '-' interface)) (Sysfs.list ()) |
There was a problem hiding this comment.
This is still doing a lookup by device name... We should not make any assumptions about the particular device name used for the VLAN. For example, I can setup a VLAN called vlanrob on top of eth0 with tag 666. Then this function won't find it, even though the VLAN is in use.
|
@robhoes I need to discuss few alternatives, will take that offline before updating PR. Thanks |
|
Maybe close this PR and re-open it when you are ready? |
|
Lets use |
| in | ||
| (* We need only device and VLAN ID from each line of config file *) | ||
| let create_pair lst = | ||
| match (Re_str.split (Re_str.regexp_string "| ") lst) with |
There was a problem hiding this comment.
I believe that the Re_str module is not thread-safe. We've had issues with it in the past. I recommend Scanf.sscanf instead:
Scanf.sscanf lst "%s | %d | %s" (fun _ device vlan -> device, vlan)
|
Thanks @robhoes for the review :) Please have a look once again. |
| let has_vlan _ dbg ~name ~vlan = | ||
| (* Identify the vlan is used by kernel which is unknown to XAPI *) | ||
| Debug.with_thread_associated dbg (fun () -> | ||
| List.mem ("", vlan, name) (Proc.get_vlans ()) |
There was a problem hiding this comment.
Let's not bother with the full_conf argument to get_vlans and always return the "full" data. You can still ignore the device name like this:
List.exists (fun (_, v, p) -> v = vlan && p = name) (Proc.get_vlans ())
| else | ||
| Scanf.sscanf lst "%s | %d | %s" (fun device vlan parent -> "", vlan, parent) | ||
| in | ||
| List.map (fun x -> create_config_list x) (List.tl (List.rev vlan_config_list)) |
There was a problem hiding this comment.
Why is the List.tl etc. needed here? To keep it simple, I would just try to parse all lines and ignore lines that cannot be parsed (and not even remove the first two lines). Also removing the full_conf parameter, something like:
let get_vlans () =
try
List.file_lines_fold (fun vlans line ->
try
let x = Scanf.sscanf line "%s | %d | %s" (fun device vlan parent -> device, vlan, parent)) in
x :: vlans
with _ ->
vlans
) [] "/proc/net/vlan/config"
with e ->
error "Error: could not read /proc/net/vlan/config";
[]
1) Function `has_vlan` will identify the VLAN which is in use by kernel and unknown to XAPI. 2) PIF.plug for VLAN PIF must fail with new exception `Vlan_in_use` if VLAN is in use by kernel and unknown to XAPI. Signed-off-by: sharad yadav <[email protected]>
|
It doesn't compile. Travis says: However, that would be due to the xcp-idl dependency. |
|
I'm going to put this in now :) |
|
Maybe because |
Function
is_vlan_in_usewill identify the VLAN which isin use by FCoE and unknown to XAPI.
Signed-off-by: sharad yadav [email protected]