CP-26856: unbind pciback driver before disable SR-IOV

[xiewei20082008]

Signed-off-by: Wei Xie wei.xie@citrix.com

---

This change is 

[xiewei20082008]

Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

lib/network_utils.ml, line 289 at r1 (raw file):

Previously, robhoes (Rob Hoes) wrote…

Why don't we just want to unbind any driver? The goal here is to switch of SR-IOV completely, so I think we can use a big hammer.

I was thinking about a case where users manually use VFs in Dom0 as a NIC, then a VF can be bound to a VF driver like igbvf. For that case, I think it is better to notify users VF %s is bound to an unexpected driver rather than to make a user find his VF NIC just disappeared.

---

lib/network_utils.ml, line 306 at r1 (raw file):

Previously, robhoes (Rob Hoes) wrote…

Nice, you're getting quite good with the `>>=` :)

Haha, 'a -> 'b -> 'a is really good pattern. :)

---

Comments from Reviewable

[robhoes]

Reviewed 3 of 3 files at r1. Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

lib/network_utils.ml, line 289 at r1 (raw file):

Previously, xiewei20082008 (Wei Xie) wrote…

I was thinking about a case where users manually use VFs in Dom0 as a NIC, then a VF can be bound to a VF driver like `igbvf`. For that case, I think it is better to notify users `VF %s is bound to an unexpected driver` rather than to make a user find his VF NIC just disappeared.

I don't think we can support such use cases. Users have to make a choice: either let xapi/networkd manage the NIC, or not, but not half/half. It would create all sorts of complications.

---

Comments from Reviewable

[xiewei20082008]

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

lib/network_utils.ml, line 269 at r1 (raw file):

Previously, mseri (Marcello Seri) wrote…

`xcp-networkd` has been ported away from `Xstringext`. Please do not use `Xstringext` and instead use ```ocaml Atring.String.is_infix ~affix:(Unix.readlink x) ```

Done. Thank you.

---

lib/network_utils.ml, line 289 at r1 (raw file):

Previously, robhoes (Rob Hoes) wrote…

I don't think we can support such use cases. Users have to make a choice: either let xapi/networkd manage the NIC, or not, but not half/half. It would create all sorts of complications.

Done. Understood.

---

lib/network_utils.ml, line 294 at r1 (raw file):

Previously, robhoes (Rob Hoes) wrote…

Instead of using the fixed `!pciback_unbind_interface`, can't we just use something like `vf_path ^ "/driver/unbind"`, which will work for pciback , and other drivers as well (usually)? I'm not sure about `remove_slot`, but we can do that as best-effort.

Done.

---

networkd/networkd.ml, line 64 at r1 (raw file):

Previously, robhoes (Rob Hoes) wrote…

Hmm... separate commits for additional fixes please. I had to stare at this for a while to see what had changed or whether it was just whitespace.

Done. Splitted it into two commits.

---

Comments from Reviewable

[robhoes]

Reviewed 2 of 2 files at r2. Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

lib/network_utils.ml, line 269 at r1 (raw file):

Previously, xiewei20082008 (Wei Xie) wrote…

Done. Thank you.

Good, looks like you've correctly Marcello's version as well (he said).

---

lib/network_utils.ml, line 290 at r2 (raw file):

          (* not bind to any driver, Ok *)
          | "" -> Result.Ok ()
          | driver -> 

This is a little confusing now, because now you have driver and driver_name for the same thing. It's better with an if-statement (if driver_name = "" then .. else ..).

---

Comments from Reviewable

[xiewei20082008]

Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

lib/network_utils.ml, line 290 at r2 (raw file):

Previously, robhoes (Rob Hoes) wrote…

This is a little confusing now, because now you have `driver` and `driver_name` for the same thing. It's better with an if-statement (`if driver_name = "" then .. else ..`).

Done.

---

Comments from Reviewable

[robhoes]

---

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[cheng-z]

Hi @robhoes and @mseri Could you please help to check if this PR can be merge now? Thanks.