Merge ~fnordahl/ubuntu/+source/libvirt:bug/1892132-focal into ubuntu/+source/libvirt:ubuntu/focal-devel

Proposed by Frode Nordahl
Status: Merged
Approved by: Christian Ehrhardt 
Approved revision: f7e021f24ee7cf0dd3dfe67a5943efa8ee033ba4
Merged at revision: f7e021f24ee7cf0dd3dfe67a5943efa8ee033ba4
Proposed branch: ~fnordahl/ubuntu/+source/libvirt:bug/1892132-focal
Merge into: ubuntu/+source/libvirt:ubuntu/focal-devel
Diff against target: 297 lines (+269/-0)
4 files modified
debian/changelog (+9/-0)
debian/patches/series (+2/-0)
debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch (+138/-0)
debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch (+120/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  (community) Approve
Canonical Server Team Pending
Ubuntu Server Dev import team Pending
Review via email: mp+405823@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

We've just completed the former libvirt SRU - well actually it still is in phasing but it can be considered complete. So it is the right time to do this.

* Changelog:
  - [x] changelog entry correct version and targeted codename
    There is
       libvirt | 6.0.0-0ubuntu8.12 | focal-proposed | source
    you'd need to rebase this

  - [+] changelog entries correct
  - [+] update-maintainer has been run

* New Delta:
  - [-] patches match what was proposed/committed upstream
  - [-] patches correctly included in debian/patches/series
  - [-] patches have correct DEP3 metadata

Both patches are adapted due to not using auto-free back then.
But both LGTM and are properly marked "Origin: backport," - thanks.

It isn't strictly required, but libvirt has that many patches that Ubuntu and started to structure them in directories.
You'll have (a subset of) directories in debian/patches for
- debian (custom patches for Debian)
- stable (pre release stable updates)
- revert (plain reverts of upstream)
- forwarded (a patch created for U/D and forwarded to upstream)
- ubuntu (custom patches for Ubuntu)
- ubuntu-aa (custom patches for Ubuntu in regard to apparmor)

Not all are sorted yet, but at least those we add we try to keep in line, therefore I'd ask to put them into debian/patches/ubuntu if that isn't too much to ask for.

In regard to metadata you have origin - thanks, that probably is the most important one.
There are many that can be added, but of the somewhat required fields you miss out on adding
Bug-Ubuntu: https://bugs.launchpad.net/bugs/<todo-bug>
Last-Update: 20xx-mm-dd

Also there is no pain, but some gain to keep the original patch description in the header.
In this case it explains a lot, no need to remove it here.

* Git/Maintenance
  - [+] testcases added or not needed for this
  - [-] commits are properly split (more important on -dev than on SRUs)

It is not strictly needed for SRUs, but still helps later maintenance (e.g. to cherry pick the changes but not the CL which always is different) if code-changes and changelog would be in individual commits. If you don't mind changing that would be appreciated.

* Build/Test:
  - not checked, is there a PPA with this somewhere already?

review: Needs Fixing
Revision history for this message
Frode Nordahl (fnordahl) wrote :

Thank you for the review, Christian!

The branch has been updated to address your insightful feedback.

The package is also building over here: https://launchpad.net/~fnordahl/+archive/ubuntu/lp-1892132

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi Frode,
thanks for v2 - this now LGTM reciew-wise.
Currently the SRU queue for libvirt is free and for testing of this path it needs your (or other peoples) special HW anyway.

The SRU template int he bug LTGM as well and I think my usual testing would not provide much benefit for this change.

The PPA build LGTM as well - thanks for the link.
(you only built amd64, but for this change that should be ok)

There is one issue left.
I asked you to bump 6.0.0-0ubuntu8.12 that you had because there already was another 6.0.0-0ubuntu8.12 - but you made it 6.0.0-0ubuntu8.12.1 while it needs to be 6.0.0-0ubuntu8.13.

I can fix this on sponsoring or do you quickly want to update your branch?

review: Needs Fixing
Revision history for this message
Frode Nordahl (fnordahl) wrote :

Hi Christian, ta for re-review, and apologies for missing the request for .13 vs .12.1.

Have now updated the commit message and changelog file to point to 6.0.0-0ubuntu8.13

Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Thanks, sponsoring now ...

review: Approve
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading libvirt_6.0.0-0ubuntu8.13.dsc: done.
  Uploading libvirt_6.0.0-0ubuntu8.13.debian.tar.xz: done.
  Uploading libvirt_6.0.0-0ubuntu8.13_source.buildinfo: done.
  Uploading libvirt_6.0.0-0ubuntu8.13_source.changes: done.
Successfully uploaded packages.

 * [new tag] upload/6.0.0-0ubuntu8.13 -> upload/6.0.0-0ubuntu8.13

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 9aa9073..7ad6fee 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,12 @@
6+libvirt (6.0.0-0ubuntu8.13) focal; urgency=medium
7+
8+ * Add support for switchdev NICs that link representor ports to parent PCI
9+ device. (LP: #1892132)
10+ - d/p/u/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
11+ - d/p/u/lp-1892132-add-virNetDevGetPhysPortName.patch
12+
13+ -- Frode Nordahl <frode.nordahl@canonical.com> Fri, 16 Jul 2021 05:16:36 +0000
14+
15 libvirt (6.0.0-0ubuntu8.12) focal; urgency=medium
16
17 * d/p/u/lp-1929202-*: fix pre-creation of images during migration
18diff --git a/debian/patches/series b/debian/patches/series
19index 814bac6..2b4f3a6 100644
20--- a/debian/patches/series
21+++ b/debian/patches/series
22@@ -167,3 +167,5 @@ ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Move-monitor-call-out-of-t.patch
23 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Use-virHashNew-and-automat.patch
24 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Remove-ret-variable-and-cl.patch
25 ubuntu/lp-1929202-qemuMigrationCookieAddNBD-Fix-filling-of-capacity-wh.patch
26+ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
27+ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
28diff --git a/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch b/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
29new file mode 100644
30index 0000000..2e468ed
31--- /dev/null
32+++ b/debian/patches/ubuntu/lp-1892132-Add-phys_port_name-support-on-virPCIGetNetName.patch
33@@ -0,0 +1,138 @@
34+Description: To support kernels or dkms builds of mlx5_core driver with
35+ https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
36+Origin: backport, https://github.com/libvirt/libvirt/commit/5b1c525b1f3608156884aed0dc5e925306c1e260
37+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1892132
38+Last-Update: 2021-08-13
39+
40+diff --git a/src/util/virpci.c b/src/util/virpci.c
41+index 0b1222373e..a54be60da0 100644
42+--- a/src/util/virpci.c
43++++ b/src/util/virpci.c
44+@@ -2424,9 +2424,9 @@ virPCIDeviceAddressGetSysfsFile(virPCIDeviceAddressPtr addr,
45+ * virPCIGetNetName:
46+ * @device_link_sysfs_path: sysfs path to the PCI device
47+ * @idx: used to choose which netdev when there are several
48+- * (ignored if physPortID is set)
49++ * (ignored if physPortID is set or physPortName is available)
50+ * @physPortID: match this string in the netdev's phys_port_id
51+- * (or NULL to ignore and use idx instead)
52++ * (or NULL to ignore and use phys_port_name or idx instead)
53+ * @netname: used to return the name of the netdev
54+ * (set to NULL (but returns success) if there is no netdev)
55+ *
56+@@ -2461,6 +2461,14 @@ virPCIGetNetName(const char *device_link_sysfs_path,
57+ }
58+
59+ while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) {
60++ /* save the first entry we find to use as a failsafe
61++ * in case we don't match the phys_port_id. This is
62++ * needed because some NIC drivers (e.g. i40e)
63++ * implement phys_port_id for PFs, but not for VFs
64++ */
65++ if (!firstEntryName)
66++ firstEntryName = g_strdup(entry->d_name);
67++
68+ /* if the caller sent a physPortID, compare it to the
69+ * physportID of this netdev. If not, look for entry[idx].
70+ */
71+@@ -2471,19 +2479,35 @@ virPCIGetNetName(const char *device_link_sysfs_path,
72+ /* if this one doesn't match, keep looking */
73+ if (STRNEQ_NULLABLE(physPortID, thisPhysPortID)) {
74+ VIR_FREE(thisPhysPortID);
75+- /* save the first entry we find to use as a failsafe
76+- * in case we don't match the phys_port_id. This is
77+- * needed because some NIC drivers (e.g. i40e)
78+- * implement phys_port_id for PFs, but not for VFs
79+- */
80+- if (!firstEntryName)
81+- firstEntryName = g_strdup(entry->d_name);
82+-
83+ continue;
84+ }
85+ } else {
86+- if (i++ < idx)
87+- continue;
88++ /* Most switch devices use phys_port_name instead of
89++ * phys_port_id.
90++ * NOTE: VFs' representors net devices can be linked to PF's PCI
91++ * device, which mean that there'll be multiple net devices
92++ * instances and to get a proper net device need to match on
93++ * specific regex.
94++ * To get PF netdev, for ex., used following regex:
95++ * "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
96++ * or to get exact VF's netdev next regex is used:
97++ * "pf0vf1$"
98++ */
99++ g_autofree char *thisPhysPortName = NULL;
100++ if (virNetDevGetPhysPortName(entry->d_name, &thisPhysPortName) < 0)
101++ goto cleanup;
102++
103++ if (thisPhysPortName) {
104++
105++ /* if this one doesn't match, keep looking */
106++ if (!virStringMatch(thisPhysPortName, VIR_PF_PHYS_PORT_NAME_REGEX))
107++ continue;
108++
109++ } else {
110++
111++ if (i++ < idx)
112++ continue;
113++ }
114+ }
115+
116+ *netname = g_strdup(entry->d_name);
117+@@ -2493,24 +2517,21 @@ virPCIGetNetName(const char *device_link_sysfs_path,
118+ }
119+
120+ if (ret < 0) {
121+- if (physPortID) {
122+- if (firstEntryName) {
123+- /* we didn't match the provided phys_port_id, but this
124+- * is probably because phys_port_id isn't implemented
125+- * for this NIC driver, so just return the first
126+- * (probably only) netname we found.
127+- */
128+- *netname = firstEntryName;
129+- firstEntryName = NULL;
130+- ret = 0;
131+- } else {
132+- virReportError(VIR_ERR_INTERNAL_ERROR,
133+- _("Could not find network device with "
134+- "phys_port_id '%s' under PCI device at %s"),
135+- physPortID, device_link_sysfs_path);
136+- }
137++ if (firstEntryName) {
138++ /* we didn't match the provided phys_port_id / find a
139++ * phys_port_name matching VIR_PF_PHYS_PORT_NAME_REGEX / find
140++ * as many net devices as the value of idx, but this is
141++ * probably because phys_port_id / phys_port_name isn't
142++ * implemented for this NIC driver, so just return the first
143++ * (probably only) netname we found.
144++ */
145++ *netname = firstEntryName;
146++ ret = 0;
147+ } else {
148+- ret = 0; /* no netdev at the given index is *not* an error */
149++ virReportError(VIR_ERR_INTERNAL_ERROR,
150++ _("Could not find any network device under PCI device at %s"),
151++ device_link_sysfs_path);
152++ ret = -1;
153+ }
154+ }
155+ cleanup:
156+diff --git a/src/util/virpci.h b/src/util/virpci.h
157+index f6796fc422..e47c766918 100644
158+--- a/src/util/virpci.h
159++++ b/src/util/virpci.h
160+@@ -49,6 +49,11 @@ struct _virZPCIDeviceAddress {
161+
162+ #define VIR_PCI_DEVICE_ADDRESS_FMT "%04x:%02x:%02x.%d"
163+
164++/* Represents format of PF's phys_port_name in switchdev mode:
165++ * 'p%u' or 'p%us%u'. New line checked since value is readed from sysfs file.
166++ */
167++#define VIR_PF_PHYS_PORT_NAME_REGEX "(p[0-9]+$)|(p[0-9]+s[0-9]+$)"
168++
169+ struct _virPCIDeviceAddress {
170+ unsigned int domain;
171+ unsigned int bus;
172diff --git a/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch b/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
173new file mode 100644
174index 0000000..757067c
175--- /dev/null
176+++ b/debian/patches/ubuntu/lp-1892132-add-virNetDevGetPhysPortName.patch
177@@ -0,0 +1,120 @@
178+Description: To support kernels or dkms builds of mlx5_core driver with
179+ https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=123f0f53dd64b67e34142485fe866a8a581f12f1
180+Origin: backport, https://github.com/libvirt/libvirt/commit/97ebb982453bc23759c5f180799d6f2207b81c80
181+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1892132
182+Last-Update: 2021-08-13
183+
184+diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
185+index e2aad07c24..52c9343d63 100644
186+--- a/src/util/virnetdev.c
187++++ b/src/util/virnetdev.c
188+@@ -1143,6 +1143,29 @@ virNetDevGetPCIDevice(const char *devName)
189+ }
190+
191+
192++/* A wrapper to get content of file from ifname SYSFS_NET_DIR
193++ */
194++static int
195++virNetDevGetSysfsFileValue(const char *ifname,
196++ const char *fileName,
197++ char **sysfsFileData)
198++{
199++ g_autofree char *sysfsFile = NULL;
200++
201++ *sysfsFileData = NULL;
202++
203++ if (virNetDevSysfsFile(&sysfsFile, ifname, fileName) < 0)
204++ return -1;
205++
206++ /* a failure to read just means the driver doesn't support
207++ * <fileName>, so set success now and ignore the return from
208++ * virFileReadAllQuiet().
209++ */
210++
211++ ignore_value(virFileReadAllQuiet(sysfsFile, 1024, sysfsFileData));
212++ return 0;
213++}
214++
215+ /**
216+ * virNetDevGetPhysPortID:
217+ *
218+@@ -1161,25 +1184,29 @@ int
219+ virNetDevGetPhysPortID(const char *ifname,
220+ char **physPortID)
221+ {
222+- int ret = -1;
223+- char *physPortIDFile = NULL;
224+-
225+- *physPortID = NULL;
226+-
227+- if (virNetDevSysfsFile(&physPortIDFile, ifname, "phys_port_id") < 0)
228+- goto cleanup;
229+-
230+- /* a failure to read just means the driver doesn't support
231+- * phys_port_id, so set success now and ignore the return from
232+- * virFileReadAllQuiet().
233+- */
234+- ret = 0;
235++ return virNetDevGetSysfsFileValue(ifname, "phys_port_id", physPortID);
236++}
237+
238+- ignore_value(virFileReadAllQuiet(physPortIDFile, 1024, physPortID));
239+
240+- cleanup:
241+- VIR_FREE(physPortIDFile);
242+- return ret;
243++/**
244++ * virNetDevGetPhysPortName:
245++ *
246++ * @ifname: name of a netdev
247++ *
248++ * @physPortName: pointer to char* that will receive @ifname's
249++ * phys_port_name from sysfs (null terminated
250++ * string). Could be NULL if @ifname's net driver doesn't
251++ * support phys_port_name (most netdev drivers
252++ * don't). Caller is responsible for freeing the string
253++ * when finished.
254++ *
255++ * Returns 0 on success or -1 on failure.
256++ */
257++int
258++virNetDevGetPhysPortName(const char *ifname,
259++ char **physPortName)
260++{
261++ return virNetDevGetSysfsFileValue(ifname, "phys_port_name", physPortName);
262+ }
263+
264+
265+@@ -1461,6 +1488,17 @@ virNetDevGetPhysPortID(const char *ifname G_GNUC_UNUSED,
266+ return 0;
267+ }
268+
269++int
270++virNetDevGetPhysPortName(const char *ifname G_GNUC_UNUSED,
271++ char **physPortName)
272++{
273++ /* this actually should never be called, and is just here to
274++ * satisfy the linker.
275++ */
276++ *physPortName = NULL;
277++ return 0;
278++}
279++
280+ int
281+ virNetDevGetVirtualFunctions(const char *pfname G_GNUC_UNUSED,
282+ char ***vfname G_GNUC_UNUSED,
283+diff --git a/src/util/virnetdev.h b/src/util/virnetdev.h
284+index 24b41498ed..26fe76cc2c 100644
285+--- a/src/util/virnetdev.h
286++++ b/src/util/virnetdev.h
287+@@ -227,6 +227,10 @@ int virNetDevGetPhysPortID(const char *ifname,
288+ char **physPortID)
289+ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
290+ G_GNUC_WARN_UNUSED_RESULT;
291++int virNetDevGetPhysPortName(const char *ifname,
292++ char **physPortName)
293++ ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2)
294++ G_GNUC_WARN_UNUSED_RESULT;
295+
296+ int virNetDevGetVirtualFunctions(const char *pfname,
297+ char ***vfname,

Subscribers

People subscribed via source and target branches