[libvirt] [PATCH 3/4] util: Fix for NULL dereference

[Radoslaw Biernacki]

The device xml parser code does not set "model" while parsing
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00' function='0x2'/>
</source>
</interface>
virDomainDefPtr def->nets[i]->model can be NULL while latter compares strings with
STREQ instead of STREQ_NULLABLE.

Fixes: ac47e4a6225 (qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound")
Fixes: c7fc151eec7 (qemu: assign virtio devices to PCIe slot when appropriate)
Signed-off-by: Radoslaw Biernacki <***@linaro.org>
---
src/qemu/qemu_domain_address.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 27c9bfb946..15d25481d8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -232,8 +232,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];

- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (STREQ_NULLABLE(net->model, "spapr-vlan")) {
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
}

@@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
virDomainNetDefPtr net = def->nets[i];

if (net->model &&
- STREQ(net->model, "virtio") &&
+ STREQ_NULLABLE(net->model, "virtio") &&
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -634,14 +633,14 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* addresses for other hostdev devices.
*/
if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ STREQ_NULLABLE(net->model, "usb-net")) {
return 0;
}

- if (STREQ(net->model, "virtio"))
+ if (STREQ_NULLABLE(net->model, "virtio"))
return virtioFlags;

- if (STREQ(net->model, "e1000e"))
+ if (STREQ_NULLABLE(net->model, "e1000e"))
return pcieFlags;

return pciFlags;

[Radoslaw Biernacki]

libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.

One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.

Signed-off-by: Radoslaw Biernacki <***@linaro.org>
---
src/util/virnetdev.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..e55c538a29 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
}

if (!*pfname) {
- /* this shouldn't be possible. A VF can't exist unless its
- * PF device is bound to a network driver
- */
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The PF device for VF %s has no network device name"),
ifname);
@@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;

- if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- goto cleanup;
+ if (is_vf == 1) {
+ /* ignore error if PF does noto have netdev assigned
+ * in that case pfname == NULL */
+ ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));
+ }

pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
virNetDevGetPCIDevice(ifname);

[Michal Privoznik]

Post by Radoslaw Biernacki
libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.
One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.
---
src/util/virnetdev.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..e55c538a29 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
}
if (!*pfname) {
- /* this shouldn't be possible. A VF can't exist unless its
- * PF device is bound to a network driver
- */
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The PF device for VF %s has no network device name"),
ifname);
@@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;
- if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- goto cleanup;
+ if (is_vf == 1) {
+ /* ignore error if PF does noto have netdev assigned
+ * in that case pfname == NULL */
+ ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));

Problem is that virNetDevGetPhysicalFunction() reports error on failure.
So either you need to take that out and put it into the other place that
calls the function (virNetDevReadNetConfig) or call virResetLastError().

Post by Radoslaw Biernacki
+ }
virNetDevGetPCIDevice(ifname);

Michal

[Radoslaw Biernacki]

Post by Radoslaw Biernacki

Post by Radoslaw Biernacki
libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.
One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.
---
src/util/virnetdev.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..e55c538a29 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1359,9 +1359,6 @@ virNetDevGetPhysicalFunction(const char *ifname,

char **pfname)

Post by Radoslaw Biernacki
}
if (!*pfname) {
- /* this shouldn't be possible. A VF can't exist unless its
- * PF device is bound to a network driver
- */
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The PF device for VF %s has no network device

name"),

Post by Radoslaw Biernacki
ifname);
@@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;
- if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- goto cleanup;
+ if (is_vf == 1) {
+ /* ignore error if PF does noto have netdev assigned
+ * in that case pfname == NULL */
+ ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));

Problem is that virNetDevGetPhysicalFunction() reports error on failure.
So either you need to take that out and put it into the other place that
calls the function (virNetDevReadNetConfig) or call virResetLastError().

Moved error reporting out of virNetDevGetPhysicalFunction().
Fortunately with 2/4 patch, only virNetDevGetVirtualFunctionInfo() calls it.
Fixed in v2.

Post by Radoslaw Biernacki

Post by Radoslaw Biernacki
+ }
virNetDevGetPCIDevice(ifname);

Michal

[Radoslaw Biernacki]

Removing redundant sections of the code

Signed-off-by: Radoslaw Biernacki <***@linaro.org>
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index e55c538a29..117ec41869 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1445,30 +1445,12 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
-
*pfname = NULL;

if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
- return ret;
-
- if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
- goto cleanup;
-
- if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
- goto cleanup;
-
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
-
- cleanup:
- if (ret < 0)
- VIR_FREE(*pfname);
-
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
+ return -1;

- return ret;
+ return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
}

#else /* !__linux__ */
@@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

if (pfDevName) {
@@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

/* if there is a PF, it's now in pfDevName, and linkdev is either
@@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

[Michal Privoznik]

Post by Radoslaw Biernacki
Removing redundant sections of the code
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)

ACK

Michal

[Radoslaw Biernacki]

linkdev is In/Out function parameter as second order reference pointer
so requires first order dereference for checking NULLs which can be a
result from virPCIGetNetName()

Fixes: d6ee56d7237 (util: change virPCIGetNetName() to not return error if device has no net name)
Signed-off-by: Radoslaw Biernacki <***@linaro.org>
---
src/util/virhostdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 1898f9eeb9..1d9345beda 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -317,7 +317,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
if (virPCIGetNetName(sysfs_path, 0, NULL, linkdev) < 0)
return -1;

- if (!linkdev) {
+ if (!(*linkdev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The device at %s has no network device name"),
sysfs_path);

[Michal Privoznik]

Post by Radoslaw Biernacki
linkdev is In/Out function parameter as second order reference pointer
so requires first order dereference for checking NULLs which can be a
result from virPCIGetNetName()
Fixes: d6ee56d7237 (util: change virPCIGetNetName() to not return error if device has no net name)
---
src/util/virhostdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

ACK

Michal

[Michal Privoznik]

Post by Radoslaw Biernacki
The device xml parser code does not set "model" while parsing
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00' function='0x2'/>
</source>
</interface>
virDomainDefPtr def->nets[i]->model can be NULL while latter compares strings with
STREQ instead of STREQ_NULLABLE.
Fixes: ac47e4a6225 (qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound")
Fixes: c7fc151eec7 (qemu: assign virtio devices to PCIe slot when appropriate)
---
src/qemu/qemu_domain_address.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 27c9bfb946..15d25481d8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -232,8 +232,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (STREQ_NULLABLE(net->model, "spapr-vlan")) {
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
}

We don't require curly braces around single line bodies. Actually the
opposite, our coding style says there should be none. This exception to
the rule was discussed many times but without any result. Anyway, 'make
syntax-check' would have caught this.

Post by Radoslaw Biernacki
@@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
virDomainNetDefPtr net = def->nets[i];
if (net->model &&
- STREQ(net->model, "virtio") &&
+ STREQ_NULLABLE(net->model, "virtio") &&

Looks like you've forgotten to remove net->model check ;-)

Post by Radoslaw Biernacki
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -634,14 +633,14 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* addresses for other hostdev devices.
*/
if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ STREQ_NULLABLE(net->model, "usb-net")) {
return 0;
}
- if (STREQ(net->model, "virtio"))
+ if (STREQ_NULLABLE(net->model, "virtio"))
return virtioFlags;
- if (STREQ(net->model, "e1000e"))
+ if (STREQ_NULLABLE(net->model, "e1000e"))
return pcieFlags;
return pciFlags;

The rest looks okay.

Michal

[Radoslaw Biernacki]

Post by Radoslaw Biernacki

Post by Radoslaw Biernacki
The device xml parser code does not set "model" while parsing
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00'

function='0x2'/>

Post by Radoslaw Biernacki
</source>
</interface>
virDomainDefPtr def->nets[i]->model can be NULL while latter compares

strings with

Post by Radoslaw Biernacki
STREQ instead of STREQ_NULLABLE.
Fixes: ac47e4a6225 (qemu: replace "def->nets[i]" with "net" and

"def->sounds[i]" with "sound")

Post by Radoslaw Biernacki
Fixes: c7fc151eec7 (qemu: assign virtio devices to PCIe slot when

appropriate)

Post by Radoslaw Biernacki
---
src/qemu/qemu_domain_address.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain_address.c

b/src/qemu/qemu_domain_address.c

Post by Radoslaw Biernacki
index 27c9bfb946..15d25481d8 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -232,8 +232,7 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr

def)

Post by Radoslaw Biernacki
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];
- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (STREQ_NULLABLE(net->model, "spapr-vlan")) {
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
}

We don't require curly braces around single line bodies. Actually the
opposite, our coding style says there should be none. This exception to
the rule was discussed many times but without any result. Anyway, 'make
syntax-check' would have caught this.

Sorry Michal, overlooked that.
Fixed in v2.

Post by Radoslaw Biernacki

Post by Radoslaw Biernacki
@@ -325,7 +324,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr

def,

Post by Radoslaw Biernacki
virDomainNetDefPtr net = def->nets[i];
if (net->model &&
- STREQ(net->model, "virtio") &&
+ STREQ_NULLABLE(net->model, "virtio") &&

Looks like you've forgotten to remove net->model check ;-)

Post by Radoslaw Biernacki
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -634,14 +633,14 @@

qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,

Post by Radoslaw Biernacki
* addresses for other hostdev devices.
*/
if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ STREQ_NULLABLE(net->model, "usb-net")) {
return 0;
}
- if (STREQ(net->model, "virtio"))
+ if (STREQ_NULLABLE(net->model, "virtio"))
return virtioFlags;
- if (STREQ(net->model, "e1000e"))
+ if (STREQ_NULLABLE(net->model, "e1000e"))
return pcieFlags;
return pciFlags;

The rest looks okay.
Michal

[Radoslaw Biernacki]

The device xml parser code does not set "model" while parsing
<interface type='hostdev'>
<source>
<address type='pci' domain='0x0002' bus='0x01' slot='0x00' function='0x2'/>
</source>
</interface>
virDomainDefPtr def->nets[i]->model can be NULL while latter compares strings with
STREQ instead of STREQ_NULLABLE.

Fixes: ac47e4a6225 (qemu: replace "def->nets[i]" with "net" and "def->sounds[i]" with "sound")
Fixes: c7fc151eec7 (qemu: assign virtio devices to PCIe slot when appropriate)
Signed-off-by: Radoslaw Biernacki <***@linaro.org>
Reviewed-by: Michal Privoznik <***@redhat.com>
---
src/qemu/qemu_domain_address.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 73ed9cc68c..f9275ed810 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -232,10 +232,8 @@ qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def)
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];

- if (net->model &&
- STREQ(net->model, "spapr-vlan")) {
+ if (STREQ_NULLABLE(net->model, "spapr-vlan"))
net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO;
- }

if (qemuDomainAssignSpaprVIOAddress(def, &net->info, VIO_ADDR_NET) < 0)
goto cleanup;
@@ -324,8 +322,7 @@ qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
for (i = 0; i < def->nnets; i++) {
virDomainNetDefPtr net = def->nets[i];

- if (net->model &&
- STREQ(net->model, "virtio") &&
+ if (STREQ_NULLABLE(net->model, "virtio") &&
net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
net->info.type = type;
}
@@ -692,14 +689,14 @@ qemuDomainDeviceCalculatePCIConnectFlags(virDomainDeviceDefPtr dev,
* addresses for other hostdev devices.
*/
if (net->type == VIR_DOMAIN_NET_TYPE_HOSTDEV ||
- STREQ(net->model, "usb-net")) {
+ STREQ_NULLABLE(net->model, "usb-net")) {
return 0;
}

- if (STREQ(net->model, "virtio"))
+ if (STREQ_NULLABLE(net->model, "virtio"))
return virtioFlags;

- if (STREQ(net->model, "e1000e"))
+ if (STREQ_NULLABLE(net->model, "e1000e"))
return pcieFlags;

return pciFlags;

[Radoslaw Biernacki]

linkdev is In/Out function parameter as second order reference pointer
so requires first order dereference for checking NULLs which can be a
result from virPCIGetNetName()

Fixes: d6ee56d7237 (util: change virPCIGetNetName() to not return error if device has no net name)
Signed-off-by: Radoslaw Biernacki <***@linaro.org>
Reviewed-by: Michal Privoznik <***@redhat.com>
---
src/util/virhostdev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c
index 1898f9eeb9..1d9345beda 100644
--- a/src/util/virhostdev.c
+++ b/src/util/virhostdev.c
@@ -317,7 +317,7 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev,
if (virPCIGetNetName(sysfs_path, 0, NULL, linkdev) < 0)
return -1;

- if (!linkdev) {
+ if (!(*linkdev)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("The device at %s has no network device name"),
sysfs_path);

[Radoslaw Biernacki]

libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.
The error report was moved outside from virNetDevGetPhysicalFunction()
and the error message changed slightly to reflect other potential
root causes of error.

One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.

Signed-off-by: Radoslaw Biernacki <***@linaro.org>
---
src/util/virnetdev.c | 22 ++++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..f1c2ba8c17 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1358,15 +1358,8 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
goto cleanup;
}

- if (!*pfname) {
- /* this shouldn't be possible. A VF can't exist unless its
- * PF device is bound to a network driver
- */
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("The PF device for VF %s has no network device name"),
- ifname);
+ if (!*pfname)
goto cleanup;
- }

ret = 0;
cleanup:
@@ -1453,8 +1446,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,

*pfname = NULL;

- if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
+ if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot get PF netdev name for VF %s"),
+ vfname);
return ret;
+ }

if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
goto cleanup;
@@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;

- if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- goto cleanup;
+ if (is_vf == 1) {
+ /* ignore error if PF does noto have netdev assigned
+ * in that case pfname == NULL */
+ ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));
+ }

pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
virNetDevGetPCIDevice(ifname);

[Michal Privoznik]

Post by Radoslaw Biernacki
libvirt wrongly assumes that VF netdev has to have the
netdev assigned to PF. There is no such requirement in SRIOV standard.
This patch change the virNetDevSwitchdevFeature() function to deal
with SRIOV devices which does not have netdev on PF. Also removes
one comment about PF netdev assumption.
The error report was moved outside from virNetDevGetPhysicalFunction()
and the error message changed slightly to reflect other potential
root causes of error.
One example of such devices is ThunderX VNIC.
By applying this change, VF device is used for virNetlinkCommand() as
it is the only netdev assigned to VNIC.
---
src/util/virnetdev.c | 22 ++++++++++----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index 5867977df4..f1c2ba8c17 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1358,15 +1358,8 @@ virNetDevGetPhysicalFunction(const char *ifname, char **pfname)
goto cleanup;
}
- if (!*pfname) {
- /* this shouldn't be possible. A VF can't exist unless its
- * PF device is bound to a network driver
- */
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("The PF device for VF %s has no network device name"),
- ifname);

If you remove this error reporting you have to make sure that all the
callers do report it (if needed).

Worse, this function has a non-Linux stub which sets errno = ENOSYS and
reports an error. Therefore I think the right solution is to keep this
error in and ..

Post by Radoslaw Biernacki
+ if (!*pfname)
goto cleanup;
- }
ret = 0;
@@ -1453,8 +1446,12 @@ virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
*pfname = NULL;
- if (virNetDevGetPhysicalFunction(vfname, pfname) < 0)
+ if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("Cannot get PF netdev name for VF %s"),
+ vfname);
return ret;
+ }
if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
goto cleanup;
@@ -3182,8 +3179,11 @@ virNetDevSwitchdevFeature(const char *ifname,
if ((is_vf = virNetDevIsVirtualFunction(ifname)) < 0)
return ret;
- if (is_vf == 1 && virNetDevGetPhysicalFunction(ifname, &pfname) < 0)
- goto cleanup;
+ if (is_vf == 1) {
+ /* ignore error if PF does noto have netdev assigned
+ * in that case pfname == NULL */
+ ignore_value(virNetDevGetPhysicalFunction(ifname, &pfname));

.. just call this function like this:

if (virNetDevGetPhysicalFunction(ifname, &pfname) < 0) {
/* Ignore error if PF does not have a netdev assigned
* in which case pfname == NULL. */
virResetLastError();
}


Sorry for not realizing this in v1.

Michal

[Radoslaw Biernacki]

Removing redundant sections of the code

Signed-off-by: Radoslaw Biernacki <***@linaro.org>
Reviewed-by: Michal Privoznik <***@redhat.com>
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)

diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index f1c2ba8c17..9318de406c 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1441,34 +1441,16 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
-
*pfname = NULL;

if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot get PF netdev name for VF %s"),
vfname);
- return ret;
+ return -1;
}

- if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
- goto cleanup;
-
- if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
- goto cleanup;
-
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
-
- cleanup:
- if (ret < 0)
- VIR_FREE(*pfname);
-
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
-
- return ret;
+ return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);
}

#else /* !__linux__ */
@@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

if (pfDevName) {
@@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

/* if there is a PF, it's now in pfDevName, and linkdev is either
@@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/

- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

[Michal Privoznik]

Post by Radoslaw Biernacki
Removing redundant sections of the code
---
src/util/virnetdev.c | 40 +++-----------------
1 file changed, 5 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
index f1c2ba8c17..9318de406c 100644
--- a/src/util/virnetdev.c
+++ b/src/util/virnetdev.c
@@ -1441,34 +1441,16 @@ int
virNetDevGetVirtualFunctionInfo(const char *vfname, char **pfname,
int *vf)
{
- char *pf_sysfs_path = NULL, *vf_sysfs_path = NULL;
- int ret = -1;
-
*pfname = NULL;
if (virNetDevGetPhysicalFunction(vfname, pfname) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Cannot get PF netdev name for VF %s"),
vfname);

Sigh, this is overwriting an error reported by
virNetDevGetPhysicalFunction(). But it is pre-existing,

Post by Radoslaw Biernacki
- return ret;
+ return -1;
}
- if (virNetDevSysfsFile(&pf_sysfs_path, *pfname, "device") < 0)
- goto cleanup;
-
- if (virNetDevSysfsFile(&vf_sysfs_path, vfname, "device") < 0)
- goto cleanup;
-
- ret = virPCIGetVirtualFunctionIndex(pf_sysfs_path, vf_sysfs_path, vf);
-
- if (ret < 0)
- VIR_FREE(*pfname);
-
- VIR_FREE(vf_sysfs_path);
- VIR_FREE(pf_sysfs_path);
-
- return ret;
+ return virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);

Just realized that previously if there was an error then *pfname was
NULL upon returning from the function. Now it might be allocated. How about:

ret = virNetDevGetVirtualFunctionIndex(*pfname, vfname, vf);

if (ret < 0)
VIR_FREE(*pfname);

return ret;

Post by Radoslaw Biernacki
}
#else /* !__linux__ */
@@ -1863,13 +1845,9 @@ virNetDevSaveNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}
if (pfDevName) {
@@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))

No, please keep the proper < 0 comparison.

Post by Radoslaw Biernacki
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}
/* if there is a PF, it's now in pfDevName, and linkdev is either
@@ -2226,13 +2200,9 @@ virNetDevSetNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
- if (virNetDevGetPhysicalFunction(linkdev, &pfDevOrig) < 0)
+ if (virNetDevGetVirtualFunctionInfo(linkdev, &pfDevOrig, &vf))

Here too.

Post by Radoslaw Biernacki
goto cleanup;
-
pfDevName = pfDevOrig;
-
- if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, &vf) < 0)
- goto cleanup;
}

Michal