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 <mpriv...@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
