Re: [libvirt] [PATCH v3] lxc: Include support to lxc version 3.0 or higher.
On 11/9/18 12:30 PM, Julio Faracco wrote:
> This patch introduce the new settings for LXC 3.0 or higher. The older
> versions keep the compatibility to deprecated settings for LXC, but
> after release 3.0, the compatibility was removed. This commit adds the
> support to the refactored settings.
>
> v1-v2: Michal's suggestions to handle differences between versions.
> v2-v3: Adding suggestions from Pino and John too.
These type of comments would go below the --- below so that they're not
part of commit history...
>
> Signed-off-by: Julio Faracco
> ---
> src/lxc/lxc_native.c | 45 +++-
> 1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/src/lxc/lxc_native.c b/src/lxc/lxc_native.c
> index cbdec723dd..d3ba12bb0e 100644
> --- a/src/lxc/lxc_native.c
> +++ b/src/lxc/lxc_native.c
[...]
> @@ -724,12 +734,13 @@ lxcIdmapWalkCallback(const char *name, virConfValuePtr
> value, void *data)
> char type;
> unsigned long start, target, count;
>
> -if (STRNEQ(name, "lxc.id_map") || !value->str)
> +/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
> +if (STRNEQ(name, "lxc.idmap") || STRNEQ(name, "lxc.id_map") ||
> !value->str)
> return 0;
This one caused lxcconf2xmltest to fail and needs to change to:
/* LXC 3.0 uses "lxc.idmap", while legacy used "lxc.id_map" */
if (STRNEQ(name, "lxc.idmap") || !value->str) {
if (!value->str || STRNEQ(name, "lxc.id_map"))
return 0;
}
The failure occurred because of the STRNEQ OR not being true (silly me
on first pass not running the tests too ;-))
>
> if (sscanf(value->str, "%c %lu %lu %lu", ,
> , , ) != 4) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid lxc.id_map: '%s'"),
> +virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid: '%s'"),
Do you mind if I alter this to:
virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid %s: '%s'"),
name, value->str);
That way the conf name string is provided like it was before
> value->str);
> return -1;
> }
> @@ -1041,19 +1052,27 @@ lxcParseConfigString(const char *config,
> if (VIR_STRDUP(vmdef->os.init, "/sbin/init") < 0)
> goto error;
>
> -if (virConfGetValueString(properties, "lxc.utsname", ) <= 0 ||
> -VIR_STRDUP(vmdef->name, value) < 0)
> -goto error;
> +if (virConfGetValueString(properties, "lxc.uts.name", ) <= 0) {
> +virResetLastError();
> +
> +/* Check for pre LXC 3.0 legacy key */
> +if (virConfGetValueString(properties, "lxc.utsname", ) <= 0)
> +goto error;
> +}
> +
I think in this case the @value needs to be restored... Previous if the
GetValueString OR the VIR_STRDUP of the value was < 0, we go to error.
Although I'm not quite sure how @value would be NULL so as to cause the
subsequent line to be executed... In any case, copying @value needs to
be done, so add:
if (VIR_STRDUP(vmdef->name, value) < 0)
goto error;
Which I can add if you agree.
With those changes,
Reviewed-by: John Ferlan
John
As a follow-up maybe adding/altering/updating the tests/lxcconf2xmldata
to include both pre and post 3.0 type data would be a good thing.
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 4/4] util: Fixing invalid error checking from virPCIGetNetname()
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
---
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);
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 0/4] util: Fixing libvirt errors on cavium/thunder-nicvf
ThunderX is Cavium SoC. This platform contain SRIOV NIC. Unlike other commonly known network devices it does not have VF functionality duplicated in its PF. PF is purely management device (on HW level). This creates several problems with existing libvirt code as in many places libvirt assumes that each VF netdev has PF netdev assigned. This patch series trying to address issues which can be easily fixed. (mostly bug fixes found while working on full featured solution) First patch in series is most important as it allows to unblock the netdev detection and use on this platform. More details about those issues can be found at: https://bugs.linaro.org/show_bug.cgi?id=3778 https://bugs.launchpad.net/charm-nova-compute/+bug/1771662 Radoslaw Biernacki (4): util: fixing wrong assumption that PF has to have netdev assigned util: Code simplification util: Fix for NULL dereference util: Fixing invalid error checking from virPCIGetNetname() src/qemu/qemu_domain_address.c | 11 ++--- src/util/virhostdev.c | 2 +- src/util/virnetdev.c | 50 3 files changed, 16 insertions(+), 47 deletions(-) -- 2.14.1 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 2/4] util: Code simplification
Removing redundant sections of the code
Signed-off-by: Radoslaw Biernacki
---
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(_sysfs_path, *pfname, "device") < 0)
-goto cleanup;
-
-if (virNetDevSysfsFile(_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, ) < 0)
+if (virNetDevGetVirtualFunctionInfo(linkdev, , ))
goto cleanup;
-
pfDevName = pfDevOrig;
-
-if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, ) < 0)
-goto cleanup;
}
if (pfDevName) {
@@ -2021,13 +1999,9 @@ virNetDevReadNetConfig(const char *linkdev, int vf,
* it to PF + VFname
*/
-if (virNetDevGetPhysicalFunction(linkdev, ) < 0)
+if (virNetDevGetVirtualFunctionInfo(linkdev, , ))
goto cleanup;
-
pfDevName = pfDevOrig;
-
-if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, ) < 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, ) < 0)
+if (virNetDevGetVirtualFunctionInfo(linkdev, , ))
goto cleanup;
-
pfDevName = pfDevOrig;
-
-if (virNetDevGetVirtualFunctionIndex(pfDevName, linkdev, ) < 0)
-goto cleanup;
}
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 1/4] util: fixing wrong assumption that PF has to have netdev assigned
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
---
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, ) < 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, ));
+}
pci_device_ptr = pfname ? virNetDevGetPCIDevice(pfname) :
virNetDevGetPCIDevice(ifname);
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH 3/4] util: Fix for NULL dereference
The device xml parser code does not set "model" while parsing
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
---
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;
--
2.14.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
