[libvirt PATCH v4] ch: Fix cloud-hypervisor version processing
Refactor the version processing logic in ch driver to support versions
from non-release cloud-hypervisor binaries. This version also supports
versions with branch prefixes in them.
Signed-off-by: Praveen K Paladugu
---
src/ch/ch_conf.c | 34 --
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c
index a8565d9537..f421af5121 100644
--- a/src/ch/ch_conf.c
+++ b/src/ch/ch_conf.c
@@ -172,6 +172,29 @@ virCHDriverConfigDispose(void *obj)
#define MIN_VERSION ((15 * 100) + (0 * 1000) + (0))
+/**
+ * chPreProcessVersionString:
+ *
+ * Returns: a pointer to numerical version without branch/commit info
+ */
+static char *
+chPreProcessVersionString(char *version)
+{
+char *tmp = strrchr(version, '/');
+
+if (tmp)
+version = tmp + 1;
+
+if (version[0] == 'v')
+version++;
+
+tmp = strchr(version, '-');
+if (tmp)
+*tmp = '\0';
+
+return version;
+}
+
int
chExtractVersion(virCHDriver *driver)
{
@@ -193,13 +216,20 @@ chExtractVersion(virCHDriver *driver)
tmp = help;
-/* expected format: cloud-hypervisor v.. */
-if ((tmp = STRSKIP(tmp, "cloud-hypervisor v")) == NULL) {
+/* Below are example version formats and expected outputs:
+ * cloud-hypervisor v32.0.0 (expected: 32.0.0)
+ * cloud-hypervisor v33.0-104-ge0e3779e-dirty (expected: 33.0)
+ * cloud-hypervisor testing/v32.0.131-1-ga5d6db5c-dirty (expected:
32.0.131)
+ */
+if ((tmp = STRSKIP(tmp, "cloud-hypervisor ")) == NULL) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("Unexpected output of cloud-hypervisor binary"));
return -1;
}
+tmp = chPreProcessVersionString(tmp);
+VIR_DEBUG("Cloud-Hypervisor version detected: %s", tmp);
+
if (virStringParseVersion(, tmp, true) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Unable to parse cloud-hypervisor version: %1$s"),
tmp);
--
2.41.0
Re: [libvirt PATCH 5/5] qemu: Implement support for vDPA block devices
On Fri, Sep 08, 2023 at 14:51:14 -0500, Jonathon Jongsma wrote:
> On 7/24/23 8:05 AM, Peter Krempa wrote:
> > As I've promised a long time ago I gave your patches some testing in
> > regards of cooperation with blockjobs and snapshots.
> >
> > Since the new version of the patches was not yet posted on the list I'm
> > replying here including my observations from testing patches from your
> > gitlab branch:
> >
> > On Tue, Jun 06, 2023 at 16:15:30 -0500, Jonathon Jongsma wrote:
> > > Requires recent qemu with support for the virtio-blk-vhost-vdpa device
> > > and the ability to pass a /dev/fdset/N path for the vdpa path (8.1.0)
> > >
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1900770
> >
> > Since this is a feature addition the 'Fixes' keyword doesn't make sense.
> > Use e.g. 'Resolves' instead.
> >
> > Additionally you're missing the DCO certification here.
> >
> > > ---
> > > src/qemu/qemu_block.c | 20 --
> > > src/qemu/qemu_domain.c | 25
> > > src/qemu/qemu_validate.c | 44 +++---
> > > tests/qemuxml2argvdata/disk-vhostvdpa.args | 35 +
> > > tests/qemuxml2argvdata/disk-vhostvdpa.xml | 21 +++
> > > tests/qemuxml2argvtest.c | 2 +
> > > 6 files changed, 139 insertions(+), 8 deletions(-)
> > > create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.args
> > > create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.xml
> >
> > [...]
> >
> > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> > > index 2f6b32e394..119e52a7d7 100644
> > > --- a/src/qemu/qemu_domain.c
> > > +++ b/src/qemu/qemu_domain.c
> > > @@ -11157,6 +11157,28 @@
> > > qemuDomainPrepareStorageSourceFDs(virStorageSource *src,
> > > }
> > > +static int
> > > +qemuDomainPrepareStorageSourceVDPA(virStorageSource *src,
> > > + qemuDomainObjPrivate *priv)
> > > +{
> > > +qemuDomainStorageSourcePrivate *srcpriv = NULL;
> > > +virStorageType actualType = virStorageSourceGetActualType(src);
> > > +int vdpafd = -1;
> > > +
> > > +if (actualType != VIR_STORAGE_TYPE_VHOST_VDPA)
> > > +return 0;
> > > +
> > > +if ((vdpafd = qemuVDPAConnect(src->path)) < 0)
> > > +return -1;
> >
> > This function call directly touches the host filesystem, which is not
> > supposed to be in the *DomainPrepareStorageSource* functions but we
> > rather have a completely separate machinery in
> > qemuProcessPrepareHostStorage.
> >
> > Unfortunately that one doesn't yet need to handle individual backing
> > chain members though.
> >
> > This ensures that the code doesn't get accidentally called from tests
> > even without mocking the code as the tests reimplement the functions
> > differently for testing purposes.
>
> Somehow I missed this comment earlier. Unfortunately, it doesn't seem
> straightforward to move this code. We can't simply move all of the logic to
> qemuProcessPrepareHostStorage() because that function doesn't get called
> during the tests.
That is the exact idea of it. This must not be called from tests because
you must not attempt to rely on host state during tests.
To make tests work, in testCompareXMLToArgvCreateArgs, we then setup a
fake FD to be passed to the command line generator. Similarly other
stuff is mocked there, mostly FD passing-related.
Additionally qemuProcessPrepareHostStorage() is also skipped inside the
domxmlToNative API as you also don't want to actually attempt opening
the VDPA socket in case when it won't be used. In that case mocking via
LD_PRELOAD is not possible.
In certain cases (if we care enough about the domxml->native API) we
even setup an alternative syntax (e.g. not using FD passing) so that the
users can adapt and use such commandline as inspiration to actually run
qemu.
> I thought I could move only the opening of the fd to the
> PrepareHostStorage() function and then keep the qemuFDPass construction in
> this function, but that doesn't work: the PrepareHostStorage() function
> actually gets called *after* this function. So the fd would not even be open
> yet at the time this function gets called.
>
> So... it seems that the options are either:
>
> - leave everything in qemuDomainPrepareStorageSourceVDPA() (as is)
No. That would violate the idea of qemuProcessPrepareHostStorage().
> - move the fd opening to PrepareHostStorage() and then move the rest to a
> different common function that is called after that, such as
> qemuBuildDiskSourceCommandLine()
Inside PrepareHostStorage() you both open the FD and create the fdpass
structure and assign it to the private data, so there's no need for any
other location to do anything else.
> - a third option (suggestions?)
>
> It's worth noting that the vdpa *network* device essentially does everything
> (opening the file, creating the qemuFDPass object, etc) in the
> qemuBuildInterfaceCommandLine() function.
Re: [libvirt PATCH 5/5] qemu: Implement support for vDPA block devices
On 7/24/23 8:05 AM, Peter Krempa wrote:
As I've promised a long time ago I gave your patches some testing in
regards of cooperation with blockjobs and snapshots.
Since the new version of the patches was not yet posted on the list I'm
replying here including my observations from testing patches from your
gitlab branch:
On Tue, Jun 06, 2023 at 16:15:30 -0500, Jonathon Jongsma wrote:
Requires recent qemu with support for the virtio-blk-vhost-vdpa device
and the ability to pass a /dev/fdset/N path for the vdpa path (8.1.0)
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1900770
Since this is a feature addition the 'Fixes' keyword doesn't make sense.
Use e.g. 'Resolves' instead.
Additionally you're missing the DCO certification here.
---
src/qemu/qemu_block.c | 20 --
src/qemu/qemu_domain.c | 25
src/qemu/qemu_validate.c | 44 +++---
tests/qemuxml2argvdata/disk-vhostvdpa.args | 35 +
tests/qemuxml2argvdata/disk-vhostvdpa.xml | 21 +++
tests/qemuxml2argvtest.c | 2 +
6 files changed, 139 insertions(+), 8 deletions(-)
create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.args
create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.xml
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2f6b32e394..119e52a7d7 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -11157,6 +11157,28 @@ qemuDomainPrepareStorageSourceFDs(virStorageSource
*src,
}
+static int
+qemuDomainPrepareStorageSourceVDPA(virStorageSource *src,
+ qemuDomainObjPrivate *priv)
+{
+qemuDomainStorageSourcePrivate *srcpriv = NULL;
+virStorageType actualType = virStorageSourceGetActualType(src);
+int vdpafd = -1;
+
+if (actualType != VIR_STORAGE_TYPE_VHOST_VDPA)
+return 0;
+
+if ((vdpafd = qemuVDPAConnect(src->path)) < 0)
+return -1;
This function call directly touches the host filesystem, which is not
supposed to be in the *DomainPrepareStorageSource* functions but we
rather have a completely separate machinery in
qemuProcessPrepareHostStorage.
Unfortunately that one doesn't yet need to handle individual backing
chain members though.
This ensures that the code doesn't get accidentally called from tests
even without mocking the code as the tests reimplement the functions
differently for testing purposes.
Somehow I missed this comment earlier. Unfortunately, it doesn't seem
straightforward to move this code. We can't simply move all of the logic
to qemuProcessPrepareHostStorage() because that function doesn't get
called during the tests. I thought I could move only the opening of the
fd to the PrepareHostStorage() function and then keep the qemuFDPass
construction in this function, but that doesn't work: the
PrepareHostStorage() function actually gets called *after* this
function. So the fd would not even be open yet at the time this function
gets called.
So... it seems that the options are either:
- leave everything in qemuDomainPrepareStorageSourceVDPA() (as is)
- move the fd opening to PrepareHostStorage() and then move the rest to
a different common function that is called after that, such as
qemuBuildDiskSourceCommandLine()
- a third option (suggestions?)
It's worth noting that the vdpa *network* device essentially does
everything (opening the file, creating the qemuFDPass object, etc) in
the qemuBuildInterfaceCommandLine() function. This was done to match
other network devices that connect to open file descriptors (see
qemuBuildInterfaceConnect()). But based on your comments above, it
sounds like this may not be a the ideal situation even though the
function is mocked to not actually open any file descriptors from the
host filesystem under test.
Jonathon
+
+srcpriv = qemuDomainStorageSourcePrivateFetch(src);
+
+srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv);
+qemuFDPassAddFD(srcpriv->fdpass, , "-vdpa");
+return 0;
+}
[...]
diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 9dce908cfe..67b0944162 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -3220,6 +3220,28 @@ qemuValidateDomainDeviceDefDiskTransient(const
virDomainDiskDef *disk,
}
+static int
+qemuValidateDomainDeviceDefDiskVhost(const virDomainDef *def,
+ virStorageType storagetype,
+ virQEMUCapsFlags flag,
+ virQEMUCaps *qemuCaps)
+{
+const char *vhosttype = virStorageTypeToString(storagetype);
+
+if (!virQEMUCapsGet(qemuCaps, flag)) {
+virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("%1$s disk is not supported with this QEMU binary"),
+ vhosttype);
+return -1;
I'd prefer if both things this function does are duplicated
[PATCH v2] interface: fix udev_device_get_sysattr_value return value check
Reviewing the code I found that return value of function
udev_device_get_sysattr_value() is dereferenced without a check.
udev_device_get_sysattr_value() may return NULL by number of reasons.
v2: VIR_DEBUG added, replaced STREQ(NULLSTR()) with STREQ_NULLABLE()
Signed-off-by: Dmitry Frolov
---
src/interface/interface_backend_udev.c | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index a0485ddd21..df7066727e 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -23,6 +23,7 @@
#include
#include
+#include "virlog.h"
#include "virerror.h"
#include "virfile.h"
#include "datatypes.h"
@@ -40,6 +41,8 @@
#define VIR_FROM_THIS VIR_FROM_INTERFACE
+VIR_LOG_INIT("interface.interface_backend_udev");
+
struct udev_iface_driver {
struct udev *udev;
/* pid file FD, ensures two copies of the driver can't use the same root */
@@ -355,10 +358,13 @@ udevConnectListAllInterfaces(virConnectPtr conn,
g_autoptr(virInterfaceDef) def = NULL;
path = udev_list_entry_get_name(dev_entry);
-dev = udev_device_new_from_syspath(udev, path);
+if (!(dev = udev_device_new_from_syspath(udev, path))) {
+VIR_DEBUG("Skipping interface '%s'", NULLSTR(path));
+continue;
+}
name = udev_device_get_sysname(dev);
macaddr = udev_device_get_sysattr_value(dev, "address");
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev,
"operstate"), "up");
def = udevGetMinimalDefForDevice(dev);
if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +970,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
/* MTU */
mtu_str = udev_device_get_sysattr_value(dev, "mtu");
-if (virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
+if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
-_("Could not parse MTU value '%1$s'"), mtu_str);
+_("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
goto error;
}
ifacedef->mtu = mtu;
@@ -1089,7 +1095,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
goto cleanup;
/* Check if it's active or not */
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ_NULLABLE(udev_device_get_sysattr_value(dev, "operstate"),
"up");
udev_device_unref(dev);
--
2.34.1
Re: [PATCH] interface: fix udev_device_get_sysattr_value return value check
On Fri, Sep 08, 2023 at 13:06:43 +0300, Dmitry Frolov wrote: > Reviewing the code I found that return value of function > udev_device_get_sysattr_value() is dereferenced without a check. > udev_device_get_sysattr_value() may return NULL by number of reasons. > > Signed-off-by: Dmitry Frolov > --- > src/interface/interface_backend_udev.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/src/interface/interface_backend_udev.c > b/src/interface/interface_backend_udev.c > index a0485ddd21..c820b3ccdf 100644 > --- a/src/interface/interface_backend_udev.c > +++ b/src/interface/interface_backend_udev.c > @@ -355,10 +355,11 @@ udevConnectListAllInterfaces(virConnectPtr conn, > g_autoptr(virInterfaceDef) def = NULL; > > path = udev_list_entry_get_name(dev_entry); > -dev = udev_device_new_from_syspath(udev, path); > +if (!(dev = udev_device_new_from_syspath(udev, path))) > +continue; IMO this warants at least a VIR_DEBUG stating that the returned 'dev' struct is NULL and thus we're skipping a whole interface. > name = udev_device_get_sysname(dev); This is also documented as returning NULL on error, but virGetInterface actually checks it before use. > macaddr = udev_device_get_sysattr_value(dev, "address"); > -status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), > "up"); > +status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, > "operstate")), "up"); For comparing strings which can be NULL we have STREQ_NULLABLE and STRNEQ_NULLABLE.
[PATCH] interface: fix udev_device_get_sysattr_value return value check
Reviewing the code I found that return value of function
udev_device_get_sysattr_value() is dereferenced without a check.
udev_device_get_sysattr_value() may return NULL by number of reasons.
Signed-off-by: Dmitry Frolov
---
src/interface/interface_backend_udev.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index a0485ddd21..c820b3ccdf 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -355,10 +355,11 @@ udevConnectListAllInterfaces(virConnectPtr conn,
g_autoptr(virInterfaceDef) def = NULL;
path = udev_list_entry_get_name(dev_entry);
-dev = udev_device_new_from_syspath(udev, path);
+if (!(dev = udev_device_new_from_syspath(udev, path)))
+continue;
name = udev_device_get_sysname(dev);
macaddr = udev_device_get_sysattr_value(dev, "address");
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev,
"operstate")), "up");
def = udevGetMinimalDefForDevice(dev);
if (!virConnectListAllInterfacesCheckACL(conn, def)) {
@@ -964,9 +965,9 @@ udevGetIfaceDef(struct udev *udev, const char *name)
/* MTU */
mtu_str = udev_device_get_sysattr_value(dev, "mtu");
-if (virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
+if (!mtu_str || virStrToLong_ui(mtu_str, NULL, 10, ) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
-_("Could not parse MTU value '%1$s'"), mtu_str);
+_("Could not parse MTU value '%1$s'"), NULLSTR(mtu_str));
goto error;
}
ifacedef->mtu = mtu;
@@ -1089,7 +1090,7 @@ udevInterfaceIsActive(virInterfacePtr ifinfo)
goto cleanup;
/* Check if it's active or not */
-status = STREQ(udev_device_get_sysattr_value(dev, "operstate"), "up");
+status = STREQ(NULLSTR(udev_device_get_sysattr_value(dev, "operstate")),
"up");
udev_device_unref(dev);
--
2.34.1
Re: [PATCH] virnetdevopenvswitch: Propagate OVS error messages
On Fri, Sep 08, 2023 at 11:04:38AM +0200, Michal Privoznik wrote:
When configuring OVS interfaces/bridges we spawn 'ovs-vsctl' with
appropriate arguments and if it exited with a non-zero status we
report a generic error message, like "Unable to add port vnet0 to
OVS bridge ovsbr0". This is all cool, but the real reason why
operation failed is hidden in (debug) logs because that's where
virCommandRun() reports it unless caller requested otherwise.
This is a bit clumsy because then we have to ask users to turn on
debug logs and reproduce the problem again, e.g. [1].
Therefore, in cases where an error is reported to the user - just
read ovs-vsctl's stderr and include it in the error message. For
other cases (like VIR_DEBUG/VIR_WARN) - well they are meant to
end up in (debug) logs anyway.
1: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-September/052640.html
Signed-off-by: Michal Privoznik
Reviewed-by: Martin Kletzander
---
src/util/virnetdevopenvswitch.c | 93 -
1 file changed, 58 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 8dad6ed2bd..d836d05845 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -54,10 +54,14 @@ virNetDevOpenvswitchSetTimeout(unsigned int timeout)
}
static virCommand *
-virNetDevOpenvswitchCreateCmd(void)
+virNetDevOpenvswitchCreateCmd(char **errbuf)
{
virCommand *cmd = virCommandNew(OVS_VSCTL);
+
virCommandAddArgFormat(cmd, "--timeout=%u", virNetDevOpenvswitchTimeout);
+if (errbuf)
+virCommandSetErrorBuffer(cmd, errbuf);
+
return cmd;
}
@@ -137,6 +141,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
char ifuuidstr[VIR_UUID_STRING_BUFLEN];
char vmuuidstr[VIR_UUID_STRING_BUFLEN];
g_autoptr(virCommand) cmd = NULL;
+g_autofree char *errbuf = NULL;
g_autofree char *attachedmac_ex_id = NULL;
g_autofree char *ifaceid_ex_id = NULL;
g_autofree char *profile_ex_id = NULL;
@@ -157,7 +162,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
ovsport->profileID);
}
-cmd = virNetDevOpenvswitchCreateCmd();
+cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--", "--may-exist",
"add-port", brname, ifname, NULL);
@@ -185,8 +190,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to add port %1$s to OVS bridge %2$s"),
- ifname, brname);
+ _("Unable to add port %1$s to OVS bridge %2$s: %3$s"),
+ ifname, brname, NULLSTR(errbuf));
return -1;
}
@@ -203,13 +208,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
*/
int virNetDevOpenvswitchRemovePort(const char *brname G_GNUC_UNUSED, const char
*ifname)
{
-g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--", "--if-exists", "del-port", ifname, NULL);
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to delete port %1$s from OVS"), ifname);
+ _("Unable to delete port %1$s from OVS: %2$s"),
+ ifname, NULLSTR(errbuf));
return -1;
}
@@ -228,7 +235,8 @@ int virNetDevOpenvswitchRemovePort(const char *brname
G_GNUC_UNUSED, const char
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
size_t len;
-g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--if-exists", "get", "Interface",
ifname, "external_ids:PortData", NULL);
@@ -238,8 +246,8 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate,
const char *ifname)
/* Run the command */
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to run command to get OVS port data for interface
%1$s"),
- ifname);
+ _("Unable to run command to get OVS port data for interface
%1$s: %2$s"),
+ ifname, NULLSTR(errbuf));
return -1;
}
@@ -263,21 +271,22 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate,
const char *ifname)
int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname)
{
g_autoptr(virCommand) cmd = NULL;
+g_autofree char *errbuf = NULL;
if (!migrate) {
VIR_DEBUG("No OVS port data for interface %s", ifname);
return 0;
}
-cmd =
[PATCH] virnetdevopenvswitch: Propagate OVS error messages
When configuring OVS interfaces/bridges we spawn 'ovs-vsctl' with
appropriate arguments and if it exited with a non-zero status we
report a generic error message, like "Unable to add port vnet0 to
OVS bridge ovsbr0". This is all cool, but the real reason why
operation failed is hidden in (debug) logs because that's where
virCommandRun() reports it unless caller requested otherwise.
This is a bit clumsy because then we have to ask users to turn on
debug logs and reproduce the problem again, e.g. [1].
Therefore, in cases where an error is reported to the user - just
read ovs-vsctl's stderr and include it in the error message. For
other cases (like VIR_DEBUG/VIR_WARN) - well they are meant to
end up in (debug) logs anyway.
1: https://mail.openvswitch.org/pipermail/ovs-discuss/2023-September/052640.html
Signed-off-by: Michal Privoznik
---
src/util/virnetdevopenvswitch.c | 93 -
1 file changed, 58 insertions(+), 35 deletions(-)
diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c
index 8dad6ed2bd..d836d05845 100644
--- a/src/util/virnetdevopenvswitch.c
+++ b/src/util/virnetdevopenvswitch.c
@@ -54,10 +54,14 @@ virNetDevOpenvswitchSetTimeout(unsigned int timeout)
}
static virCommand *
-virNetDevOpenvswitchCreateCmd(void)
+virNetDevOpenvswitchCreateCmd(char **errbuf)
{
virCommand *cmd = virCommandNew(OVS_VSCTL);
+
virCommandAddArgFormat(cmd, "--timeout=%u", virNetDevOpenvswitchTimeout);
+if (errbuf)
+virCommandSetErrorBuffer(cmd, errbuf);
+
return cmd;
}
@@ -137,6 +141,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
char ifuuidstr[VIR_UUID_STRING_BUFLEN];
char vmuuidstr[VIR_UUID_STRING_BUFLEN];
g_autoptr(virCommand) cmd = NULL;
+g_autofree char *errbuf = NULL;
g_autofree char *attachedmac_ex_id = NULL;
g_autofree char *ifaceid_ex_id = NULL;
g_autofree char *profile_ex_id = NULL;
@@ -157,7 +162,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
ovsport->profileID);
}
-cmd = virNetDevOpenvswitchCreateCmd();
+cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--", "--may-exist",
"add-port", brname, ifname, NULL);
@@ -185,8 +190,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to add port %1$s to OVS bridge %2$s"),
- ifname, brname);
+ _("Unable to add port %1$s to OVS bridge %2$s: %3$s"),
+ ifname, brname, NULLSTR(errbuf));
return -1;
}
@@ -203,13 +208,15 @@ int virNetDevOpenvswitchAddPort(const char *brname, const
char *ifname,
*/
int virNetDevOpenvswitchRemovePort(const char *brname G_GNUC_UNUSED, const
char *ifname)
{
-g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--", "--if-exists", "del-port", ifname, NULL);
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to delete port %1$s from OVS"), ifname);
+ _("Unable to delete port %1$s from OVS: %2$s"),
+ ifname, NULLSTR(errbuf));
return -1;
}
@@ -228,7 +235,8 @@ int virNetDevOpenvswitchRemovePort(const char *brname
G_GNUC_UNUSED, const char
int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname)
{
size_t len;
-g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
+g_autofree char *errbuf = NULL;
+g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd();
virCommandAddArgList(cmd, "--if-exists", "get", "Interface",
ifname, "external_ids:PortData", NULL);
@@ -238,8 +246,8 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate,
const char *ifname)
/* Run the command */
if (virCommandRun(cmd, NULL) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
- _("Unable to run command to get OVS port data for
interface %1$s"),
- ifname);
+ _("Unable to run command to get OVS port data for
interface %1$s: %2$s"),
+ ifname, NULLSTR(errbuf));
return -1;
}
@@ -263,21 +271,22 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate,
const char *ifname)
int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname)
{
g_autoptr(virCommand) cmd = NULL;
+g_autofree char *errbuf = NULL;
if (!migrate) {
VIR_DEBUG("No OVS port data for interface %s", ifname);
return 0;
}
-cmd = virNetDevOpenvswitchCreateCmd();
+cmd =
