[libvirt] [PATCH 0/5] qemu: Fix guestfwd hot(un-)plug
*** BLURB HERE *** Michal Prívozník (5): qemuDomainDetachChrDevice: Use @tmpChr to build device string qemuL: Drop "user-" prefix for guestfwd netdev qemu_hotplug: Attach guestfwd using netdev_add qemu_hotplug: Detach guestfwd using netdev_del qemuhotplugtest: Test guestfwd attach and detach src/qemu/qemu_command.c | 2 +- src/qemu/qemu_hotplug.c | 70 +++++++++++++------ tests/qemuhotplugtest.c | 6 ++ .../qemuhotplug-guestfwd.xml | 4 ++ .../qemuhotplug-base-live+guestfwd.xml | 55 +++++++++++++++ tests/qemuxml2argvdata/channel-guestfwd.args | 2 +- .../channel-unix-guestfwd.x86_64-2.5.0.args | 4 +- .../channel-unix-guestfwd.x86_64-latest.args | 4 +- tests/qemuxml2argvdata/name-escape.args | 2 +- 9 files changed, 121 insertions(+), 28 deletions(-) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml -- 2.19.2
So far we are passing @chr to qemuBuildChrDeviceStr. This is suboptimal beacuse @chr is just parsed XML definition provided by user which by definition may lack some information. On the other hand, @tmpChr is the one that was found using @chr in domain definition so it contains the same amount of information or more. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 38600b33f8..01920a78f5 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -6131,7 +6131,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, sa_assert(tmpChr->info.alias); - if (qemuBuildChrDeviceStr(&devstr, vmdef, chr, priv->qemuCaps) < 0) + if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup; if (!async) -- 2.19.2
$SUBJ: qemu: Use @tmpChr in qemuDomainDetachChrDevice to build device string On 2/11/19 10:40 AM, Michal Privoznik wrote:
So far we are passing @chr to qemuBuildChrDeviceStr. This is suboptimal beacuse @chr is just parsed XML definition provided by
because
user which by definition may lack some information. On the other hand, @tmpChr is the one that was found using @chr in domain definition so it contains the same amount of information or more.
It's more than suboptimal, it ends up being wrong since 24b082192 - looks like a copy-pasta error to me from Attach code... I'd probably just go with we need to be sure to use the @tmpChr definition we just found via virDomainChrFind since it contains more details used when creating the command line that we'll need when rebuilding that command line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
Introduced by d86c876a66e3. There is no real need to have "user-" prefix for chardev. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/channel-guestfwd.args | 2 +- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args | 4 ++-- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args | 4 ++-- tests/qemuxml2argvdata/name-escape.args | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 572d3bc20f..c20d8b02a9 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -10857,7 +10857,7 @@ qemuBuildChannelChrDeviceStr(char **deviceStr, port = virSocketAddrGetPort(chr->target.addr); if (virAsprintf(deviceStr, - "user,guestfwd=tcp:%s:%i-chardev:char%s,id=user-%s", + "user,guestfwd=tcp:%s:%i-chardev:char%s,id=%s", addr, port, chr->info.alias, chr->info.alias) < 0) goto cleanup; break; diff --git a/tests/qemuxml2argvdata/channel-guestfwd.args b/tests/qemuxml2argvdata/channel-guestfwd.args index a82d5b6d4e..a5e9a12520 100644 --- a/tests/qemuxml2argvdata/channel-guestfwd.args +++ b/tests/qemuxml2argvdata/channel-guestfwd.args @@ -24,5 +24,5 @@ server,nowait \ -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ -device ide-hd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=1 \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd \ --netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ +-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args b/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args index e8776ca24e..da062968ff 100644 --- a/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args +++ b/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args @@ -24,8 +24,8 @@ server,nowait \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -chardev socket,id=charchannel0,path=/tmp/guestfwd-listen.socket,server,nowait \ --netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ +-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ -chardev socket,id=charchannel1,path=/tmp/guestfwd-connect.socket \ --netdev user,guestfwd=tcp:10.0.2.1:4601-chardev:charchannel1,id=user-channel1 \ +-netdev user,guestfwd=tcp:10.0.2.1:4601-chardev:charchannel1,id=channel1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -msg timestamp=on diff --git a/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args b/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args index db14217c1b..34a3d33f27 100644 --- a/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args +++ b/tests/qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args @@ -25,9 +25,9 @@ file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ -boot strict=on \ -device piix3-usb-uhci,id=usb,bus=pci.0,addr=0x1.0x2 \ -chardev socket,id=charchannel0,fd=1729,server,nowait \ --netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ +-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ -chardev socket,id=charchannel1,path=/tmp/guestfwd-connect.socket \ --netdev user,guestfwd=tcp:10.0.2.1:4601-chardev:charchannel1,id=user-channel1 \ +-netdev user,guestfwd=tcp:10.0.2.1:4601-chardev:charchannel1,id=channel1 \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x2 \ -sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,\ resourcecontrol=deny \ diff --git a/tests/qemuxml2argvdata/name-escape.args b/tests/qemuxml2argvdata/name-escape.args index b4cad57602..9190b0b921 100644 --- a/tests/qemuxml2argvdata/name-escape.args +++ b/tests/qemuxml2argvdata/name-escape.args @@ -36,7 +36,7 @@ cert3=cert3,db=/etc/pki/nssdb,,foo,id=smartcard0,bus=ccid0.0 \ -chardev file,id=charserial1,path=/tmp/serial.log,,foo,append=on \ -device isa-serial,chardev=charserial1,id=serial1 \ -chardev pipe,id=charchannel0,path=/tmp/guestfwd,,foo \ --netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=user-channel0 \ +-netdev user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0 \ -vnc unix:/tmp/lib/domain--1-foo=1,,bar=2/vnc.sock \ -spice unix,addr=/tmp/lib/domain--1-foo=1,,bar=2/spice.sock,gl=on,\ rendernode=/dev/dri/foo,,bar,seamless-migration=on \ -- 2.19.2
$SUBJ: s/qemuL:/qemu:/ On 2/11/19 10:40 AM, Michal Privoznik wrote:
Introduced by d86c876a66e3.
There is no real need to have "user-" prefix for chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/channel-guestfwd.args | 2 +- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args | 4 ++-- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args | 4 ++-- tests/qemuxml2argvdata/name-escape.args | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
At least it wasn't "ua-" ;-) Although I don't believe this patch is necessary. It's only made necessary because two patches from now qemuDomainDetachChrDevice wants to use tmpChr->info.alias instead of grabbing the alias that was used to generated the command line from qemuBuildChrDeviceStr out of the @devstr. Let me hold off on an R-by for this until you give feedback on my patch4 comments... John BTW: looks to me like af249ea46 originally implemented using id= string without the user- prefix. Eventually 7b2f8cdd7c removed id=, but then the above restores it; however, w/ user- prefix.
On 2/12/19 11:08 PM, John Ferlan wrote:
$SUBJ:
s/qemuL:/qemu:/
On 2/11/19 10:40 AM, Michal Privoznik wrote:
Introduced by d86c876a66e3.
There is no real need to have "user-" prefix for chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/channel-guestfwd.args | 2 +- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-2.5.0.args | 4 ++-- .../qemuxml2argvdata/channel-unix-guestfwd.x86_64-latest.args | 4 ++-- tests/qemuxml2argvdata/name-escape.args | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
At least it wasn't "ua-" ;-)
Although I don't believe this patch is necessary. It's only made necessary because two patches from now qemuDomainDetachChrDevice wants to use tmpChr->info.alias instead of grabbing the alias that was used to generated the command line from qemuBuildChrDeviceStr out of the @devstr.
Let me hold off on an R-by for this until you give feedback on my patch4 comments...
Given that guestfwd has a very limitted use case and that as you pointed out we switched it multiple times, there shouldn't be much problems with this. I rather be unable to detach guestfwd than come up with some complicated changes to our code just to allow it for domains started with older libvirt. Michal
https://bugzilla.redhat.com/show_bug.cgi?id=1624204 The guestfwd channels are -netdevs really. Hotplug them as such. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 01920a78f5..a0ccc3b82c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2292,10 +2292,14 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, char *tlsAlias = NULL; const char *secAlias = NULL; bool need_release = false; + bool guestfwd = false; - if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && - qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) - goto cleanup; + if (chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL) { + guestfwd = chr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + + if (qemuDomainPrepareChannel(chr, priv->channelTargetDir) < 0) + goto cleanup; + } if (qemuAssignDeviceChrAlias(vmdef, chr, -1) < 0) goto cleanup; @@ -2337,8 +2341,14 @@ int qemuDomainAttachChrDevice(virQEMUDriverPtr driver, goto exit_monitor; chardevAttached = true; - if (qemuMonitorAddDevice(priv->mon, devstr) < 0) - goto exit_monitor; + if (guestfwd) { + if (qemuMonitorAddNetdev(priv->mon, devstr, + NULL, NULL, 0, NULL, NULL, 0) < 0) + goto exit_monitor; + } else { + if (qemuMonitorAddDevice(priv->mon, devstr) < 0) + goto exit_monitor; + } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto audit; -- 2.19.2
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotplug them as such.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-)
NB: Nothing in this patch requires patch2 - I can remove patch2 and this works without it. The fix is of course the qemuMonitorAddNetdev. Reviewed-by: John Ferlan <jferlan@redhat.com> John
https://bugzilla.redhat.com/show_bug.cgi?id=1624204 The guestfwd channels are -netdevs really. Hotunplug them as such. Also, DEVICE_DELETED event is not triggered (surprisingly, since we're not issuing device_del rather than netdev_del) and associated chardev is removed automagically too. This means that we need to do qemuDomainRemoveChrDevice() minus monitor call to remove the chardev. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a0ccc3b82c..107d0fb7a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool monitor) { virObjectEventPtr event; char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int rc; + int rc = 0; VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); - if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) - goto cleanup; + if (monitor) { + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) + goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + } if (rc == 0 && qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; + bool guestfwd = false; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; } + /* guestfwd channels are not really -device rather than + * -netdev. We need to treat them slightly differently. */ + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup; @@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup; - if (!async) + if (!async && !guestfwd) qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup; + if (guestfwd) { + if (qemuMonitorRemoveNetdev(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + } else { + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; - if (async) { + if (guestfwd) { + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, false); + } else if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, true); } cleanup: -- 2.19.2
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as such. Also, DEVICE_DELETED event is not triggered (surprisingly, since we're not issuing device_del rather than netdev_del) and associated chardev is removed automagically too. This means that we need to do qemuDomainRemoveChrDevice() minus monitor call to remove the chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going to be able to remove any guestfwd that is already assigned to a guest because it'll have that "user-" prefix... It will work once the guest is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a0ccc3b82c..107d0fb7a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool monitor) { virObjectEventPtr event; char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int rc; + int rc = 0;
VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name);
- if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) - goto cleanup; + if (monitor) { + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) + goto cleanup;
- qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + }
if (rc == 0 && qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break;
case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; + bool guestfwd = false;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; }
+ /* guestfwd channels are not really -device rather than + * -netdev. We need to treat them slightly differently. */ + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup;
@@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup;
Bright idea... The returned @devstr contains the "id=" value used to generate the command line, thus why not extract out the id= value to be the alias used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice? That way patch2 becomes unnecessary (I tried it). Of course fear always strikes my soul when "assuming" anything, so I figured I'd run it past you first for your thoughts. I am OK with the patches as they are except for the tiny gotcha of never being able to remove that user-channel# device.
- if (!async) + if (!async && !guestfwd) qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup;
Hmm... If one considers that if @devstr were NULL, then this code would do nothing, then what is it's purpose? There is no other consumer of qemuBuildChrDeviceStr that could believe @devstr == NULL and ret == 0 that I could find. Thoughts before an R-by? Tks - John
+ if (guestfwd) { + if (qemuMonitorRemoveNetdev(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } + } else { + if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { + ignore_value(qemuDomainObjExitMonitor(driver, vm)); + goto cleanup; + } } if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup;
- if (async) { + if (guestfwd) { + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, false); + } else if (async) { ret = 0; } else { if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1) - ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr); + ret = qemuDomainRemoveChrDevice(driver, vm, tmpChr, true); }
cleanup:
On 2/12/19 11:19 PM, John Ferlan wrote:
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as such. Also, DEVICE_DELETED event is not triggered (surprisingly, since we're not issuing device_del rather than netdev_del) and associated chardev is removed automagically too. This means that we need to do qemuDomainRemoveChrDevice() minus monitor call to remove the chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going to be able to remove any guestfwd that is already assigned to a guest because it'll have that "user-" prefix... It will work once the guest is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a0ccc3b82c..107d0fb7a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool monitor) { virObjectEventPtr event; char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int rc; + int rc = 0;
VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name);
- if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) - goto cleanup; + if (monitor) { + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) + goto cleanup;
- qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias);
- if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + }
if (rc == 0 && qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break;
case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; + bool guestfwd = false;
if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; }
+ /* guestfwd channels are not really -device rather than + * -netdev. We need to treat them slightly differently. */ + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup;
@@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup;
Bright idea...
The returned @devstr contains the "id=" value used to generate the command line, thus why not extract out the id= value to be the alias used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
Problem with this approach is that string processing in C is pain in the #@$. Cutting out the alias from @devstr would be not trivial. But okay, there's another approach - have yet another local variable that would be initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might override it for those types of chardev that need it. But this is rather ugly.
That way patch2 becomes unnecessary (I tried it). Of course fear always strikes my soul when "assuming" anything, so I figured I'd run it past you first for your thoughts. I am OK with the patches as they are except for the tiny gotcha of never being able to remove that user-channel# device.
Well, one way to look at it might be: if you have a domain started via older libvirt nothing changes for you. You're unable to detach guestfwd just like you always was :-) Then again, guestfwd really makes sense only with slirp (that's also why qemu rejects any other IP than 10.0.2.1) and since we're not setting other attributes for the netdev, the defaults are used. I mean, -netdev user,net=172.17.2.0/24,ipv6-net=2001:db8:ac10:fd01::/64,\ guestfwd=tcp:172.17.2.4:4600-chardev:charchannel1,id=channel1 is the way to set different address for guestfwd. But so far, we are not setting net= nor ipv6-net= for guestfwd <channel/> (although, we are doing that for <interface type='user'/>). Therefore I think, no one is using guestfwd really. That's why I am willing to take the risk. Having said that, I'm not sure how to fix the lack of attributes described above and thus allowing users to specify a different address. I mean, we can extend <channel/> for new attributes and just put them on the cmd line, but then it is effectively not a channel but an <interface/> (more specifically <interface type='user/> which shows up even in guest. Or we could try to pair guestfwd channels with <interface type='user'/> but the question is using what key? The Nth guestfwd would correspond do Nth <interface type='user'/>? This link would not be apparent from domain XML. Looks like we're doomed here.
- if (!async) + if (!async && !guestfwd) qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info);
qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup;
Hmm... If one considers that if @devstr were NULL, then this code would do nothing, then what is it's purpose? There is no other consumer of qemuBuildChrDeviceStr that could believe @devstr == NULL and ret == 0 that I could find.
Looks like an old artefact. And looking at the commit 24b0821926e that introduced it (oh boy, I was so young once) and indeed it is. Back in 2013 when there was no DEVICE_DELETED event libvirt detached both front- and backend at the same time. Well, this check in question was there to remove frontend only for those chardevs that had one. Michal
On 2/13/19 5:58 AM, Michal Privoznik wrote:
On 2/12/19 11:19 PM, John Ferlan wrote:
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as such. Also, DEVICE_DELETED event is not triggered (surprisingly, since we're not issuing device_del rather than netdev_del) and associated chardev is removed automagically too. This means that we need to do qemuDomainRemoveChrDevice() minus monitor call to remove the chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going to be able to remove any guestfwd that is already assigned to a guest because it'll have that "user-" prefix... It will work once the guest is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a0ccc3b82c..107d0fb7a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool monitor) { virObjectEventPtr event; char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int rc; + int rc = 0; VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); - if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) - goto cleanup; + if (monitor) { + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) + goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + } if (rc == 0 && qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; + bool guestfwd = false; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; } + /* guestfwd channels are not really -device rather than + * -netdev. We need to treat them slightly differently. */ + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup; @@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup;
Bright idea...
The returned @devstr contains the "id=" value used to generate the command line, thus why not extract out the id= value to be the alias used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
Problem with this approach is that string processing in C is pain in the #@$. Cutting out the alias from @devstr would be not trivial. But okay, there's another approach - have yet another local variable that would be initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might override it for those types of chardev that need it. But this is rather ugly.
It's not that bad... size_t i; const char *alias; VIR_AUTOPTR(elems) = NULL; then after &devstr is generated if (!(elems = virStringSplit(devstr, "," 0))) goto cleanup; for (i = 0; elems[i]; i++) { if ((alias = STRSKIP(elems[i], "id="))) break; if (!alias) { alias = tmpChr->info.alias VIR_WARN("Cannot find 'id=' in devstr='%s' using '%'s", devstr, alias); } BTW: w/r/t whether tmpChr->info.alias checking should be done consider commits c86735e2d and 2bd9db96 which I think removed it under the assumption it had better be there. I have a recollection of sending patches at some time in an effort to remove the sa_assert, but alas too much time has passed for total recall.
That way patch2 becomes unnecessary (I tried it). Of course fear always strikes my soul when "assuming" anything, so I figured I'd run it past you first for your thoughts. I am OK with the patches as they are except for the tiny gotcha of never being able to remove that user-channel# device.
Well, one way to look at it might be: if you have a domain started via older libvirt nothing changes for you. You're unable to detach guestfwd just like you always was :-)
Not sure I agree... As a test, I started a guest with a libvirt prior to your patches that had the <channel> from the BZ defined in the guest. Then I stopped libvirtd, ran my debug libvirtd using the above hunk, and I was able to detach using the guestfwd.xml from the BZ.
Then again, guestfwd really makes sense only with slirp (that's also why qemu rejects any other IP than 10.0.2.1) and since we're not setting other attributes for the netdev, the defaults are used. I mean,
-netdev user,net=172.17.2.0/24,ipv6-net=2001:db8:ac10:fd01::/64,\ guestfwd=tcp:172.17.2.4:4600-chardev:charchannel1,id=channel1
is the way to set different address for guestfwd. But so far, we are not setting net= nor ipv6-net= for guestfwd <channel/> (although, we are doing that for <interface type='user'/>). Therefore I think, no one is using guestfwd really. That's why I am willing to take the risk.
Like I did note I'm OK with the approach taken with the patches. I don't know enough about guestfwd history and/or usage model which is why I asked and made sure to note striking fear in my soul over assuming how things are working ;-) In testing with your patches, I did find I could only do at most one attach/detach cycle - a second attach for a running guest results in: error: internal error: unable to execute QEMU command 'chardev-add': attempt to add duplicate property 'charchannel0' to object (type 'container') I suspect QE would try this multiple attach/detach processing - whether it's supportable is a different issue/question. Your patches fix the 1 time approach through. If that's all that's supposed to be supported, then fine I'm not opposed to your approach. It just comes with the caveat that one is unable to detach from a guest running before and the inability to do multiple attach/detach.
Having said that, I'm not sure how to fix the lack of attributes described above and thus allowing users to specify a different address. I mean, we can extend <channel/> for new attributes and just put them on the cmd line, but then it is effectively not a channel but an <interface/> (more specifically <interface type='user/> which shows up even in guest.
Marching into territory beyond the scope... I'm not sure whether your question is on attach or detach, but I can say @devstr for the example from the BZ comes back with the following (with your patch to remove "user-" from the alias): 'user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0' grabbing out the guestfwd=, determining 'tcp' (or whatever), address, and port would seem to be easy enough at least for IPv4.
Or we could try to pair guestfwd channels with <interface type='user'/> but the question is using what key? The Nth guestfwd would correspond do Nth <interface type='user'/>? This link would not be apparent from domain XML. Looks like we're doomed here.
- if (!async) + if (!async && !guestfwd) qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup;
Hmm... If one considers that if @devstr were NULL, then this code would do nothing, then what is it's purpose? There is no other consumer of qemuBuildChrDeviceStr that could believe @devstr == NULL and ret == 0 that I could find.
Looks like an old artefact. And looking at the commit 24b0821926e that introduced it (oh boy, I was so young once) and indeed it is. Back in 2013 when there was no DEVICE_DELETED event libvirt detached both front- and backend at the same time. Well, this check in question was there to remove frontend only for those chardevs that had one.
So feel free to remove it as a trivial prepatch to make life easier. Depending on the answer to the multiple attach/detach noted above removing @devstr I believe would make patch1 obsolete. A way too long way of saying, I'm OK with your approach. Although perhaps need an answer regarding the multiple attach/detach expectations before proceeding with an R-by. John
On 2/13/19 2:39 PM, John Ferlan wrote:
On 2/13/19 5:58 AM, Michal Privoznik wrote:
On 2/12/19 11:19 PM, John Ferlan wrote:
On 2/11/19 10:40 AM, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1624204
The guestfwd channels are -netdevs really. Hotunplug them as such. Also, DEVICE_DELETED event is not triggered (surprisingly, since we're not issuing device_del rather than netdev_del) and associated chardev is removed automagically too. This means that we need to do qemuDomainRemoveChrDevice() minus monitor call to remove the chardev.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_hotplug.c | 48 ++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-)
So while, yes this does work and fix the issue... It's still not going to be able to remove any guestfwd that is already assigned to a guest because it'll have that "user-" prefix... It will work once the guest is restarted of course... so...
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a0ccc3b82c..107d0fb7a9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4742,25 +4742,28 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver, static int qemuDomainRemoveChrDevice(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainChrDefPtr chr) + virDomainChrDefPtr chr, + bool monitor) { virObjectEventPtr event; char *charAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; - int rc; + int rc = 0; VIR_DEBUG("Removing character device %s from domain %p %s", chr->info.alias, vm, vm->def->name); - if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) - goto cleanup; + if (monitor) { + if (!(charAlias = qemuAliasChardevFromDevAlias(chr->info.alias))) + goto cleanup; - qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorDetachCharDev(priv->mon, charAlias); + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorDetachCharDev(priv->mon, charAlias); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto cleanup; + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto cleanup; + } if (rc == 0 && qemuDomainDelChardevTLSObjects(driver, vm, chr->source, charAlias) < 0) @@ -5064,7 +5067,7 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver, break; case VIR_DOMAIN_DEVICE_CHR: - ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr); + ret = qemuDomainRemoveChrDevice(driver, vm, dev->data.chr, true); break; case VIR_DOMAIN_DEVICE_RNG: ret = qemuDomainRemoveRNGDevice(driver, vm, dev->data.rng); @@ -6127,6 +6130,7 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, virDomainDefPtr vmdef = vm->def; virDomainChrDefPtr tmpChr; char *devstr = NULL; + bool guestfwd = false; if (!(tmpChr = virDomainChrFind(vmdef, chr))) { virReportError(VIR_ERR_DEVICE_MISSING, @@ -6136,6 +6140,11 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, goto cleanup; } + /* guestfwd channels are not really -device rather than + * -netdev. We need to treat them slightly differently. */ + guestfwd = tmpChr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL && + tmpChr->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD; + if (!tmpChr->info.alias && qemuAssignDeviceChrAlias(vmdef, tmpChr, -1) < 0) goto cleanup; @@ -6144,22 +6153,31 @@ int qemuDomainDetachChrDevice(virQEMUDriverPtr driver, if (qemuBuildChrDeviceStr(&devstr, vmdef, tmpChr, priv->qemuCaps) < 0) goto cleanup;
Bright idea...
The returned @devstr contains the "id=" value used to generate the command line, thus why not extract out the id= value to be the alias used below for either qemuMonitorRemoveNetdev or qemuMonitorDelDevice?
Problem with this approach is that string processing in C is pain in the #@$. Cutting out the alias from @devstr would be not trivial. But okay, there's another approach - have yet another local variable that would be initialized to tmpChr->info.alias and then qemuBuildChrDeviceStr() might override it for those types of chardev that need it. But this is rather ugly.
It's not that bad...
size_t i; const char *alias; VIR_AUTOPTR(elems) = NULL;
then after &devstr is generated
if (!(elems = virStringSplit(devstr, "," 0))) goto cleanup;
for (i = 0; elems[i]; i++) { if ((alias = STRSKIP(elems[i], "id="))) break;
if (!alias) { alias = tmpChr->info.alias VIR_WARN("Cannot find 'id=' in devstr='%s' using '%'s", devstr, alias); }
BTW: w/r/t whether tmpChr->info.alias checking should be done consider commits c86735e2d and 2bd9db96 which I think removed it under the assumption it had better be there. I have a recollection of sending patches at some time in an effort to remove the sa_assert, but alas too much time has passed for total recall.
That way patch2 becomes unnecessary (I tried it). Of course fear always strikes my soul when "assuming" anything, so I figured I'd run it past you first for your thoughts. I am OK with the patches as they are except for the tiny gotcha of never being able to remove that user-channel# device.
Well, one way to look at it might be: if you have a domain started via older libvirt nothing changes for you. You're unable to detach guestfwd just like you always was :-)
Not sure I agree... As a test, I started a guest with a libvirt prior to your patches that had the <channel> from the BZ defined in the guest. Then I stopped libvirtd, ran my debug libvirtd using the above hunk, and I was able to detach using the guestfwd.xml from the BZ.
Then again, guestfwd really makes sense only with slirp (that's also why qemu rejects any other IP than 10.0.2.1) and since we're not setting other attributes for the netdev, the defaults are used. I mean,
-netdev user,net=172.17.2.0/24,ipv6-net=2001:db8:ac10:fd01::/64,\ guestfwd=tcp:172.17.2.4:4600-chardev:charchannel1,id=channel1
is the way to set different address for guestfwd. But so far, we are not setting net= nor ipv6-net= for guestfwd <channel/> (although, we are doing that for <interface type='user'/>). Therefore I think, no one is using guestfwd really. That's why I am willing to take the risk.
Like I did note I'm OK with the approach taken with the patches. I don't know enough about guestfwd history and/or usage model which is why I asked and made sure to note striking fear in my soul over assuming how things are working ;-)
In testing with your patches, I did find I could only do at most one attach/detach cycle - a second attach for a running guest results in:
error: internal error: unable to execute QEMU command 'chardev-add': attempt to add duplicate property 'charchannel0' to object (type 'container')
Interesting, this test scenario works for me just fine. What's your qemu version? Mine's v3.1.0-1709-g0b5e750bea; Looks like 3.1.0 is broken - it does not removes the chardev. And running bisect shows this was changed in e47f81b617684c4546af286d307b69014a83538a (merged Feb 7). If I use unfixed qemu, the chardev is not removed on netdev_del, but trying to remove it manually fails too: {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' is busy"}} So if users are using any qemu but the one from git, no matter what libvirt does it won't help them. Might as well go with 2/5 and require freshly start domain.
I suspect QE would try this multiple attach/detach processing - whether it's supportable is a different issue/question.
Your patches fix the 1 time approach through. If that's all that's supposed to be supported, then fine I'm not opposed to your approach. It just comes with the caveat that one is unable to detach from a guest running before and the inability to do multiple attach/detach.
Having said that, I'm not sure how to fix the lack of attributes described above and thus allowing users to specify a different address. I mean, we can extend <channel/> for new attributes and just put them on the cmd line, but then it is effectively not a channel but an <interface/> (more specifically <interface type='user/> which shows up even in guest.
Marching into territory beyond the scope... I'm not sure whether your question is on attach or detach, but I can say @devstr for the example from the BZ comes back with the following (with your patch to remove "user-" from the alias):
'user,guestfwd=tcp:10.0.2.1:4600-chardev:charchannel0,id=channel0'
grabbing out the guestfwd=, determining 'tcp' (or whatever), address, and port would seem to be easy enough at least for IPv4.
Or we could try to pair guestfwd channels with <interface type='user'/> but the question is using what key? The Nth guestfwd would correspond do Nth <interface type='user'/>? This link would not be apparent from domain XML. Looks like we're doomed here.
- if (!async) + if (!async && !guestfwd) qemuDomainMarkDeviceForRemoval(vm, &tmpChr->info); qemuDomainObjEnterMonitor(driver, vm); - if (devstr && qemuMonitorDelDevice(priv->mon, tmpChr->info.alias) < 0) { - ignore_value(qemuDomainObjExitMonitor(driver, vm)); - goto cleanup;
Hmm... If one considers that if @devstr were NULL, then this code would do nothing, then what is it's purpose? There is no other consumer of qemuBuildChrDeviceStr that could believe @devstr == NULL and ret == 0 that I could find.
Looks like an old artefact. And looking at the commit 24b0821926e that introduced it (oh boy, I was so young once) and indeed it is. Back in 2013 when there was no DEVICE_DELETED event libvirt detached both front- and backend at the same time. Well, this check in question was there to remove frontend only for those chardevs that had one.
So feel free to remove it as a trivial prepatch to make life easier. Depending on the answer to the multiple attach/detach noted above removing @devstr I believe would make patch1 obsolete.
A way too long way of saying, I'm OK with your approach. Although perhaps need an answer regarding the multiple attach/detach expectations before proceeding with an R-by.
John
Michal
[...]
In testing with your patches, I did find I could only do at most one attach/detach cycle - a second attach for a running guest results in:
error: internal error: unable to execute QEMU command 'chardev-add': attempt to add duplicate property 'charchannel0' to object (type 'container')
Interesting, this test scenario works for me just fine. What's your qemu version? Mine's v3.1.0-1709-g0b5e750bea; Looks like 3.1.0 is broken - it does not removes the chardev. And running bisect shows this was changed in e47f81b617684c4546af286d307b69014a83538a (merged Feb 7). If I use unfixed qemu, the chardev is not removed on netdev_del, but trying to remove it manually fails too:
{"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' is busy"}}
So if users are using any qemu but the one from git, no matter what libvirt does it won't help them. Might as well go with 2/5 and require freshly start domain.
Well now... Nice detective work. So I had v3.0.0 in my (new) f29 laptop - I hadn't recently done a build from qemu source and run (explains an error on another guest I had where I was using a built qemu tree based on the 2.12 branch and the guest wasn't starting). So using a top of tree QEMU allows things to succeed now for me. Probably want to be sure to update the bz with what you found so that the history isn't lost and to note the problem for QE so that they don't file a bug against libvirt on it ;-)... So let's think about this series again... Patch1 - I think perhaps it could be replaced by removing @devstr since it's really not even used. It could also be a follow-up, IDC. If you go with the former, taken an implied R-by since @devstr doesn't need to be formulated. Then for Patch2 and Patch4 - you have my Reviewed-by: John Ferlan <jferlan@redhat.com> for the patches as is. John [...]
Previous two commits demonstrate a hole in our test scenario. Fix that. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuhotplugtest.c | 6 ++ .../qemuhotplug-guestfwd.xml | 4 ++ .../qemuhotplug-base-live+guestfwd.xml | 55 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml diff --git a/tests/qemuhotplugtest.c b/tests/qemuhotplugtest.c index 6d70d5897f..1491c214d0 100644 --- a/tests/qemuhotplugtest.c +++ b/tests/qemuhotplugtest.c @@ -796,6 +796,12 @@ mymain(void) DO_TEST_DETACH("base-live", "watchdog-user-alias-full", false, false, "device_del", QMP_DEVICE_DELETED("ua-UserWatchdog") QMP_OK); + DO_TEST_ATTACH("base-live", "guestfwd", false, true, + "chardev-add", QMP_OK, + "netdev_add", QMP_OK); + DO_TEST_DETACH("base-live", "guestfwd", false, false, + "netdev_del", QMP_OK); + #define DO_TEST_CPU_GROUP(prefix, vcpus, modernhp, expectfail) \ do { \ cpudata.test = prefix; \ diff --git a/tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml b/tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml new file mode 100644 index 0000000000..c67dbdb8df --- /dev/null +++ b/tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml @@ -0,0 +1,4 @@ +<channel type='unix'> + <source mode='bind' path='/tmp/guestfwd'/> + <target type='guestfwd' address='10.0.2.1' port='4600'/> +</channel> diff --git a/tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml new file mode 100644 index 0000000000..8d7294123b --- /dev/null +++ b/tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml @@ -0,0 +1,55 @@ +<domain type='kvm' id='7'> + <name>hotplug</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <currentMemory unit='KiB'>4194304</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <features> + <acpi/> + <apic/> + <pae/> + </features> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <controller type='usb' index='0'> + <alias name='usb'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <alias name='ide'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <controller type='scsi' index='0' model='virtio-scsi'> + <alias name='scsi0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <alias name='pci'/> + </controller> + <controller type='virtio-serial' index='0'> + <alias name='virtio-serial0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </controller> + <channel type='unix'> + <source mode='bind' path='/tmp/guestfwd'/> + <target type='guestfwd' address='10.0.2.1' port='4600'/> + <alias name='channel0'/> + </channel> + <input type='mouse' bus='ps2'> + <alias name='input0'/> + </input> + <input type='keyboard' bus='ps2'> + <alias name='input1'/> + </input> + <memballoon model='none'/> + </devices> + <seclabel type='none' model='none'/> +</domain> -- 2.19.2
On 2/11/19 10:40 AM, Michal Privoznik wrote:
Previous two commits demonstrate a hole in our test scenario. Fix that.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/qemuhotplugtest.c | 6 ++ .../qemuhotplug-guestfwd.xml | 4 ++ .../qemuhotplug-base-live+guestfwd.xml | 55 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 tests/qemuhotplugtestdevices/qemuhotplug-guestfwd.xml create mode 100644 tests/qemuhotplugtestdomains/qemuhotplug-base-live+guestfwd.xml
Obviously R-by here depends on decision on patch2 and patch4. The output for "<alias name='channel0'/>" depends on the decision, but I'll give the R-by under the assumption we agree on the previous question. Reviewed-by: John Ferlan <jferlan@redhat.com> John
participants (2)
-
John Ferlan -
Michal Privoznik