[libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del
Michal Privoznik
mprivozn at redhat.com
Wed Feb 13 16:15:55 UTC 2019
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 at 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
More information about the libvir-list
mailing list