[libvirt] [PATCH 0/3] Fix regression with vhostuser and chardev FD passing
QEMU has a validation bug which prevents vhostuser accepting the chardev when it uses FD passing, despite it being functionally correct. While the QEMU bug is being fixed, we need to avoid hitting this problem with existing QEMU releases. The easiest way todo this is to simply disable FD passing for the chardev used with vhostuser only. Daniel P. Berrangé (3): qemu: remove chardevStdioLogd param from vhostuser code path qemu: consolidate parameters of qemuBuildChrChardevStr into flags qemu: don't use chardev FD passing for vhostuser backend src/qemu/qemu_command.c | 117 ++++++++++++++-------- tests/qemuxml2argvdata/net-vhostuser.args | 3 +- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 75 insertions(+), 47 deletions(-) -- 2.17.1
The vhostuser network backend is only supported with the UNIX domain socket chardev backend, so passing around chardevStdioLogd is not required. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 04c5c28438..9351b9fddb 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -8195,8 +8195,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, virDomainDefPtr def, virDomainNetDefPtr net, virQEMUCapsPtr qemuCaps, - unsigned int bootindex, - bool chardevStdioLogd) + unsigned int bootindex) { virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); char *chardev = NULL; @@ -8217,7 +8216,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, cmd, cfg, def, net->data.vhostuser, net->info.alias, qemuCaps, false, - chardevStdioLogd))) + false))) goto cleanup; break; @@ -8291,8 +8290,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, virNetDevVPortProfileOp vmop, bool standalone, size_t *nnicindexes, - int **nicindexes, - bool chardevStdioLogd) + int **nicindexes) { int ret = -1; char *nic = NULL, *host = NULL; @@ -8415,8 +8413,7 @@ qemuBuildInterfaceCommandLine(virQEMUDriverPtr driver, case VIR_DOMAIN_NET_TYPE_VHOSTUSER: ret = qemuBuildVhostuserCommandLine(driver, logManager, secManager, cmd, def, - net, qemuCaps, bootindex, - chardevStdioLogd); + net, qemuCaps, bootindex); goto cleanup; break; @@ -8600,8 +8597,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver, bool standalone, size_t *nnicindexes, int **nicindexes, - unsigned int *bootHostdevNet, - bool chardevStdioLogd) + unsigned int *bootHostdevNet) { size_t i; int last_good_net = -1; @@ -8628,8 +8624,7 @@ qemuBuildNetCommandLine(virQEMUDriverPtr driver, if (qemuBuildInterfaceCommandLine(driver, logManager, secManager, cmd, def, net, qemuCaps, bootNet, vmop, standalone, nnicindexes, - nicindexes, - chardevStdioLogd) < 0) + nicindexes) < 0) goto error; last_good_net = i; @@ -10290,8 +10285,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, if (qemuBuildNetCommandLine(driver, logManager, secManager, cmd, def, qemuCaps, vmop, standalone, - nnicindexes, nicindexes, &bootHostdevNet, - chardevStdioLogd) < 0) + nnicindexes, nicindexes, &bootHostdevNet) < 0) goto error; if (qemuBuildSmartcardCommandLine(logManager, secManager, cmd, cfg, def, qemuCaps, -- 2.17.1
There are two boolean parameters passed to qemuBuildChrChardevStr, and soon there will be a third. It will be clearer to understand from callers' POV if we use named flags instead. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 86 ++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 31 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9351b9fddb..7f3eccc1bd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4935,6 +4935,12 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev) return -1; } + +enum { + QEMU_BUILD_CHARDEV_TCP_NOWAIT = (1 << 0), + QEMU_BUILD_CHARDEV_FILE_LOGD = (1 << 1), +}; + /* This function outputs a -chardev command line option which describes only the * host side of the character device */ static char * @@ -4946,8 +4952,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, const virDomainChrSourceDef *dev, const char *alias, virQEMUCapsPtr qemuCaps, - bool nowait, - bool chardevStdioLogd) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; @@ -4986,7 +4991,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("append not supported in this QEMU binary")); goto cleanup; } - if (qemuBuildChrChardevFileStr(chardevStdioLogd ? logManager : NULL, + if (qemuBuildChrChardevFileStr(flags & QEMU_BUILD_CHARDEV_FILE_LOGD ? + logManager : NULL, cmd, def, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) @@ -5033,7 +5039,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, telnet ? ",telnet" : ""); if (dev->data.tcp.listen) - virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); + virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? + ",server,nowait" : ",server", -1); qemuBuildChrChardevReconnectStr(&buf, &dev->data.tcp.reconnect); @@ -5092,7 +5099,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); } if (dev->data.nix.listen) - virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); + virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? + ",server,nowait" : ",server", -1); qemuBuildChrChardevReconnectStr(&buf, &dev->data.nix.reconnect); break; @@ -5426,6 +5434,9 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, qemuDomainObjPrivatePtr priv) { char *chrdev; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (priv->chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; if (!priv->monConfig) return 0; @@ -5433,8 +5444,7 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, if (!(chrdev = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, priv->monConfig, "monitor", - priv->qemuCaps, true, - priv->chardevStdioLogd))) + priv->qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, chrdev); @@ -5559,6 +5569,9 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager, char **chr, bool chardevStdioLogd) { + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; *chr = NULL; switch ((virDomainRNGBackend) rng->backend) { @@ -5571,8 +5584,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager, if (!(*chr = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, rng->source.chardev, - rng->info.alias, qemuCaps, true, - chardevStdioLogd))) + rng->info.alias, qemuCaps, + cdevflags))) return -1; } @@ -8215,8 +8228,7 @@ qemuBuildVhostuserCommandLine(virQEMUDriverPtr driver, if (!(chardev = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, net->data.vhostuser, - net->info.alias, qemuCaps, false, - false))) + net->info.alias, qemuCaps, 0))) goto cleanup; break; @@ -8696,6 +8708,9 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, virBuffer opt = VIR_BUFFER_INITIALIZER; const char *database; const char *contAlias = NULL; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; if (!def->nsmartcards) return 0; @@ -8761,8 +8776,7 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, cmd, cfg, def, smartcard->data.passthru, smartcard->info.alias, - qemuCaps, true, - chardevStdioLogd))) { + qemuCaps, cdevflags))) { virBufferFreeAndReset(&opt); return -1; } @@ -8930,6 +8944,9 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, virBuffer buf = VIR_BUFFER_INITIALIZER; char *devstr = NULL; int rc; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; if (shmem->size) { /* @@ -8993,8 +9010,8 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, devstr = qemuBuildChrChardevStr(logManager, secManager, cmd, cfg, def, &shmem->server.chr, - shmem->info.alias, qemuCaps, true, - chardevStdioLogd); + shmem->info.alias, qemuCaps, + cdevflags); if (!devstr) return -1; @@ -9087,6 +9104,9 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, { size_t i; bool havespice = false; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; if (def->nserials) { for (i = 0; i < def->ngraphics && !havespice; i++) { @@ -9106,8 +9126,7 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, cmd, cfg, def, serial->source, serial->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9148,6 +9167,9 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; for (i = 0; i < def->nparallels; i++) { virDomainChrDefPtr parallel = def->parallels[i]; @@ -9157,8 +9179,7 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager, cmd, cfg, def, parallel->source, parallel->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9183,6 +9204,9 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; for (i = 0; i < def->nchannels; i++) { virDomainChrDefPtr channel = def->channels[i]; @@ -9194,8 +9218,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, cmd, cfg, def, channel->source, channel->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9212,8 +9235,7 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, cmd, cfg, def, channel->source, channel->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9239,6 +9261,9 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; /* Explicit console devices */ for (i = 0; i < def->nconsoles; i++) { @@ -9257,8 +9282,7 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, cmd, cfg, def, console->source, console->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9279,8 +9303,7 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, cmd, cfg, def, console->source, console->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9295,8 +9318,7 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, cmd, cfg, def, console->source, console->info.alias, - qemuCaps, true, - chardevStdioLogd))) + qemuCaps, cdevflags))) return -1; virCommandAddArg(cmd, "-chardev"); virCommandAddArg(cmd, devstr); @@ -9419,6 +9441,9 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + if (chardevStdioLogd) + cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; for (i = 0; i < def->nredirdevs; i++) { virDomainRedirdevDefPtr redirdev = def->redirdevs[i]; @@ -9428,8 +9453,7 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager, cmd, cfg, def, redirdev->source, redirdev->info.alias, - qemuCaps, true, - chardevStdioLogd))) { + qemuCaps, cdevflags))) { return -1; } -- 2.17.1
On 07/05/2018 01:36 PM, Daniel P. Berrangé wrote:
There are two boolean parameters passed to qemuBuildChrChardevStr, and soon there will be a third. It will be clearer to understand from callers' POV if we use named flags instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 86 ++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9351b9fddb..7f3eccc1bd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4935,6 +4935,12 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev) return -1; }
+ +enum { + QEMU_BUILD_CHARDEV_TCP_NOWAIT = (1 << 0), + QEMU_BUILD_CHARDEV_FILE_LOGD = (1 << 1), +}; + /* This function outputs a -chardev command line option which describes only the * host side of the character device */ static char * @@ -4946,8 +4952,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, const virDomainChrSourceDef *dev, const char *alias, virQEMUCapsPtr qemuCaps, - bool nowait, - bool chardevStdioLogd) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; @@ -4986,7 +4991,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("append not supported in this QEMU binary")); goto cleanup; } - if (qemuBuildChrChardevFileStr(chardevStdioLogd ? logManager : NULL, + if (qemuBuildChrChardevFileStr(flags & QEMU_BUILD_CHARDEV_FILE_LOGD ? + logManager : NULL, cmd, def, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) @@ -5033,7 +5039,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, telnet ? ",telnet" : "");
if (dev->data.tcp.listen) - virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); + virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? + ",server,nowait" : ",server", -1);
Well, since you're touching this: s/virBufferAdd/virBufferAddLit/ and also perhaps get rid of the ternary operator? diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 7f3eccc1bd..63c7ac0f82 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -5038,9 +5038,11 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, dev->data.tcp.service, telnet ? ",telnet" : ""); - if (dev->data.tcp.listen) - virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? - ",server,nowait" : ",server", -1); + if (dev->data.tcp.listen) { + virBufferAddLit(&buf, ",server"); + if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + virBufferAddLit(&buf, ",nowait"); + } qemuBuildChrChardevReconnectStr(&buf, &dev->data.tcp.reconnect); @@ -5098,9 +5100,11 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); } - if (dev->data.nix.listen) - virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? - ",server,nowait" : ",server", -1); + if (dev->data.nix.listen) { + virBufferAddLit(&buf, ",server"); + if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + virBufferAddLit(&buf, ",nowait"); + } qemuBuildChrChardevReconnectStr(&buf, &dev->data.nix.reconnect); break; Michal
On Thu, Jul 05, 2018 at 02:43:18PM +0200, Michal Prívozník wrote:
On 07/05/2018 01:36 PM, Daniel P. Berrangé wrote:
There are two boolean parameters passed to qemuBuildChrChardevStr, and soon there will be a third. It will be clearer to understand from callers' POV if we use named flags instead.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 86 ++++++++++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 31 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9351b9fddb..7f3eccc1bd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4935,6 +4935,12 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev) return -1; }
+ +enum { + QEMU_BUILD_CHARDEV_TCP_NOWAIT = (1 << 0), + QEMU_BUILD_CHARDEV_FILE_LOGD = (1 << 1), +}; + /* This function outputs a -chardev command line option which describes only the * host side of the character device */ static char * @@ -4946,8 +4952,7 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, const virDomainChrSourceDef *dev, const char *alias, virQEMUCapsPtr qemuCaps, - bool nowait, - bool chardevStdioLogd) + unsigned int flags) { virBuffer buf = VIR_BUFFER_INITIALIZER; bool telnet; @@ -4986,7 +4991,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, _("append not supported in this QEMU binary")); goto cleanup; } - if (qemuBuildChrChardevFileStr(chardevStdioLogd ? logManager : NULL, + if (qemuBuildChrChardevFileStr(flags & QEMU_BUILD_CHARDEV_FILE_LOGD ? + logManager : NULL, cmd, def, &buf, "path", dev->data.file.path, "append", dev->data.file.append) < 0) @@ -5033,7 +5039,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, telnet ? ",telnet" : "");
if (dev->data.tcp.listen) - virBufferAdd(&buf, nowait ? ",server,nowait" : ",server", -1); + virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? + ",server,nowait" : ",server", -1);
Well, since you're touching this: s/virBufferAdd/virBufferAddLit/ and also perhaps get rid of the ternary operator?
diff --git i/src/qemu/qemu_command.c w/src/qemu/qemu_command.c index 7f3eccc1bd..63c7ac0f82 100644 --- i/src/qemu/qemu_command.c +++ w/src/qemu/qemu_command.c @@ -5038,9 +5038,11 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, dev->data.tcp.service, telnet ? ",telnet" : "");
- if (dev->data.tcp.listen) - virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? - ",server,nowait" : ",server", -1); + if (dev->data.tcp.listen) { + virBufferAddLit(&buf, ",server"); + if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + virBufferAddLit(&buf, ",nowait"); + }
qemuBuildChrChardevReconnectStr(&buf, &dev->data.tcp.reconnect);
@@ -5098,9 +5100,11 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, virBufferAsprintf(&buf, "socket,id=%s,path=", charAlias); virQEMUBuildBufferEscapeComma(&buf, dev->data.nix.path); } - if (dev->data.nix.listen) - virBufferAdd(&buf, flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT ? - ",server,nowait" : ",server", -1); + if (dev->data.nix.listen) { + virBufferAddLit(&buf, ",server"); + if (flags & QEMU_BUILD_CHARDEV_TCP_NOWAIT) + virBufferAddLit(&buf, ",nowait"); + }
qemuBuildChrChardevReconnectStr(&buf, &dev->data.nix.reconnect); break;
Sure, this is fine. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
QEMU chardevs have a bug which makes the vhostuser backend complain about lack of support for FD passing when validating the chardev. While this is ultimately QEMU's responsibility to fix, libvirt needs to avoid tickling the bug. Simply disabling chardev FD passing just for vhostuser's chardev is the most prudent approach, avoiding need for a QEMU version number check. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/qemu/qemu_command.c | 31 +++++++++++++++-------- tests/qemuxml2argvdata/net-vhostuser.args | 3 +-- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 7f3eccc1bd..aaa6d0ceda 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4939,6 +4939,7 @@ qemuOpenChrChardevUNIXSocket(const virDomainChrSourceDef *dev) enum { QEMU_BUILD_CHARDEV_TCP_NOWAIT = (1 << 0), QEMU_BUILD_CHARDEV_FILE_LOGD = (1 << 1), + QEMU_BUILD_CHARDEV_UNIX_FD_PASS = (1 << 2), }; /* This function outputs a -chardev command line option which describes only the @@ -5080,7 +5081,8 @@ qemuBuildChrChardevStr(virLogManagerPtr logManager, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) { + if ((flags & QEMU_BUILD_CHARDEV_UNIX_FD_PASS) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_CHARDEV_FD_PASS)) { if (qemuSecuritySetSocketLabel(secManager, (virDomainDefPtr)def) < 0) goto cleanup; int fd = qemuOpenChrChardevUNIXSocket(dev); @@ -5434,7 +5436,8 @@ qemuBuildMonitorCommandLine(virLogManagerPtr logManager, qemuDomainObjPrivatePtr priv) { char *chrdev; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (priv->chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -5569,7 +5572,8 @@ qemuBuildRNGBackendChrdevStr(virLogManagerPtr logManager, char **chr, bool chardevStdioLogd) { - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; *chr = NULL; @@ -8708,7 +8712,8 @@ qemuBuildSmartcardCommandLine(virLogManagerPtr logManager, virBuffer opt = VIR_BUFFER_INITIALIZER; const char *database; const char *contAlias = NULL; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -8944,7 +8949,8 @@ qemuBuildShmemCommandLine(virLogManagerPtr logManager, virBuffer buf = VIR_BUFFER_INITIALIZER; char *devstr = NULL; int rc; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -9104,7 +9110,8 @@ qemuBuildSerialCommandLine(virLogManagerPtr logManager, { size_t i; bool havespice = false; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -9167,7 +9174,8 @@ qemuBuildParallelsCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -9204,7 +9212,8 @@ qemuBuildChannelsCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -9261,7 +9270,8 @@ qemuBuildConsoleCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; @@ -9441,7 +9451,8 @@ qemuBuildRedirdevCommandLine(virLogManagerPtr logManager, bool chardevStdioLogd) { size_t i; - unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT; + unsigned int cdevflags = QEMU_BUILD_CHARDEV_TCP_NOWAIT | + QEMU_BUILD_CHARDEV_UNIX_FD_PASS; if (chardevStdioLogd) cdevflags |= QEMU_BUILD_CHARDEV_FILE_LOGD; diff --git a/tests/qemuxml2argvdata/net-vhostuser.args b/tests/qemuxml2argvdata/net-vhostuser.args index fc4557a1f2..513fc535ab 100644 --- a/tests/qemuxml2argvdata/net-vhostuser.args +++ b/tests/qemuxml2argvdata/net-vhostuser.args @@ -14,8 +14,7 @@ QEMU_AUDIO_DRV=none \ -display none \ -no-user-config \ -nodefaults \ --chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ -server,nowait \ +-chardev socket,id=charmonitor,fd=1729,server,nowait \ -mon chardev=charmonitor,id=monitor,mode=control \ -rtc base=utc \ -no-shutdown \ diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d6911f9344..2d52f352b0 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1266,7 +1266,7 @@ mymain(void) DO_TEST("misc-no-reboot", NONE); DO_TEST("misc-uuid", NONE); DO_TEST_PARSE_ERROR("vhost_queues-invalid", NONE); - DO_TEST("net-vhostuser", NONE); + DO_TEST("net-vhostuser", QEMU_CAPS_CHARDEV_FD_PASS); DO_TEST("net-vhostuser-multiq", QEMU_CAPS_VHOSTUSER_MULTIQUEUE); DO_TEST_FAILURE("net-vhostuser-multiq", NONE); -- 2.17.1
On 07/05/2018 01:36 PM, Daniel P. Berrangé wrote:
QEMU has a validation bug which prevents vhostuser accepting the chardev when it uses FD passing, despite it being functionally correct.
While the QEMU bug is being fixed, we need to avoid hitting this problem with existing QEMU releases.
The easiest way todo this is to simply disable FD passing for the chardev used with vhostuser only.
Daniel P. Berrangé (3): qemu: remove chardevStdioLogd param from vhostuser code path qemu: consolidate parameters of qemuBuildChrChardevStr into flags qemu: don't use chardev FD passing for vhostuser backend
src/qemu/qemu_command.c | 117 ++++++++++++++-------- tests/qemuxml2argvdata/net-vhostuser.args | 3 +- tests/qemuxml2argvtest.c | 2 +- 3 files changed, 75 insertions(+), 47 deletions(-)
ACK series. Michal
participants (2)
-
Daniel P. Berrangé -
Michal Prívozník