[libvirt] [PATCH] conf: Parse guestfwd channel device info again

Michal Privoznik posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/3d3e7a2772108f9f8aecc0fb70950a5dbe38c587.1534783952.git.mprivozn@redhat.com
Test syntax-check passed
src/conf/domain_conf.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
[libvirt] [PATCH] conf: Parse guestfwd channel device info again
Posted by Michal Privoznik 5 years, 7 months ago
https://bugzilla.redhat.com/show_bug.cgi?id=1610072

Due to historical reasons we were not parsing device info on
guestfwd channel. Sure, it doesn't make much sense to parse
<address/> but it surely makes sense to parse its alias (which
might be an user alias).

This reverts commit 47a3dd46ead20e6fdc30bcdc1b8e707e250d33da
which fixed https://bugzilla.redhat.com/show_bug.cgi?id=1172526.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/conf/domain_conf.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3c254801cd..123abec1ba 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -12807,14 +12807,8 @@ virDomainChrDefParseXML(virDomainXMLOptionPtr xmlopt,
         }
     }
 
-    if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
-        def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
-        VIR_DEBUG("Ignoring device address for gustfwd channel");
-    } else if (virDomainDeviceInfoParseXML(xmlopt, node,
-                                           &def->info, flags) < 0) {
+    if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
         goto error;
-    }
-
 
     if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL &&
         def->targetType == VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_USB &&
-- 
2.16.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Parse guestfwd channel device info again
Posted by Andrea Bolognani 5 years, 7 months ago
On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1610072
> 
> Due to historical reasons we were not parsing device info on

You spelled "hysterical raisins" wrong ;)

[...]
> -    if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> -        def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
> -        VIR_DEBUG("Ignoring device address for gustfwd channel");
> -    } else if (virDomainDeviceInfoParseXML(xmlopt, node,
> -                                           &def->info, flags) < 0) {
> +    if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
>          goto error;
> -    }
> -

I agree that fixing Bug 1610072 is more important than preventing
Bug 1172526 from showing up again, but it would be great if we could
make it so both are fixed...

How about parsing the info and then clearing out the address only if
it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
a bit too thorough, but perhaps you can introduce a new
virDomainDeviceInfoClearAddress() that only zeroes out the address
and use that here.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Parse guestfwd channel device info again
Posted by Andrea Bolognani 5 years, 7 months ago
On Tue, 2018-08-21 at 11:01 +0200, Andrea Bolognani wrote:
> On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
> [...]
> > -    if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
> > -        def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
> > -        VIR_DEBUG("Ignoring device address for gustfwd channel");
> > -    } else if (virDomainDeviceInfoParseXML(xmlopt, node,
> > -                                           &def->info, flags) < 0) {
> > +    if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
> >          goto error;
> > -    }
> > -
> 
> I agree that fixing Bug 1610072 is more important than preventing
> Bug 1172526 from showing up again, but it would be great if we could
> make it so both are fixed...
> 
> How about parsing the info and then clearing out the address only if
> it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
> a bit too thorough, but perhaps you can introduce a new
> virDomainDeviceInfoClearAddress() that only zeroes out the address
> and use that here.

Given the issues with the approach I proposed, let's just cut our
losses and merge your original attempt instead. Sorry for wasting
your time :(

Reviewed-by: Andrea Bolognani <abologna@redhat.com>

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] conf: Parse guestfwd channel device info again
Posted by Michal Privoznik 5 years, 7 months ago
On 08/21/2018 02:15 PM, Andrea Bolognani wrote:
> On Tue, 2018-08-21 at 11:01 +0200, Andrea Bolognani wrote:
>> On Mon, 2018-08-20 at 18:52 +0200, Michal Privoznik wrote:
>> [...]
>>> -    if (def->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CHANNEL &&
>>> -        def->targetType == VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_GUESTFWD) {
>>> -        VIR_DEBUG("Ignoring device address for gustfwd channel");
>>> -    } else if (virDomainDeviceInfoParseXML(xmlopt, node,
>>> -                                           &def->info, flags) < 0) {
>>> +    if (virDomainDeviceInfoParseXML(xmlopt, node, &def->info, flags) < 0)
>>>          goto error;
>>> -    }
>>> -
>>
>> I agree that fixing Bug 1610072 is more important than preventing
>> Bug 1172526 from showing up again, but it would be great if we could
>> make it so both are fixed...
>>
>> How about parsing the info and then clearing out the address only if
>> it's a guestfwd channel? The existing virDomainDeviceInfoClear() is
>> a bit too thorough, but perhaps you can introduce a new
>> virDomainDeviceInfoClearAddress() that only zeroes out the address
>> and use that here.
> 
> Given the issues with the approach I proposed, let's just cut our
> losses and merge your original attempt instead. Sorry for wasting
> your time :(
> 

No worries.

> Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 

Pushed thanks.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list