[libvirt] [PATCH 4/5] qemu_hotplug: Detach guestfwd using netdev_del

[Michal Privoznik]

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.

> 
> 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