1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/5] chardev source reconnect cleanup and fixes
  4. View patch

[libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode

Pavel Hrdina posted 5 patches 7 years, 8 months ago
[libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Posted by Pavel Hrdina 7 years, 8 months ago 
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 src/conf/domain_conf.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index f7574d77b6..7f443e5b4d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
             virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'",
                               def->data.tcp.tlsFromConfig);
 
-        virDomainChrSourceReconnectDefFormat(&childBuf,
-                                             &def->data.tcp.reconnect);
+        if (!def->data.tcp.listen)
+            virDomainChrSourceReconnectDefFormat(&childBuf,
+                                                 &def->data.tcp.reconnect);
 
         if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
             goto error;
@@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
             virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels,
                                              def->seclabels, flags);
 
-            virDomainChrSourceReconnectDefFormat(&childBuf,
-                                                 &def->data.nix.reconnect);
+            if (!def->data.nix.listen)
+                virDomainChrSourceReconnectDefFormat(&childBuf,
+                                                     &def->data.nix.reconnect);
 
             if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
                 goto error;
-- 
2.13.5

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Posted by Michal Privoznik 7 years, 8 months ago 
On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  src/conf/domain_conf.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index f7574d77b6..7f443e5b4d 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>              virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'",
>                                def->data.tcp.tlsFromConfig);
>  
> -        virDomainChrSourceReconnectDefFormat(&childBuf,
> -                                             &def->data.tcp.reconnect);
> +        if (!def->data.tcp.listen)
> +            virDomainChrSourceReconnectDefFormat(&childBuf,
> +                                                 &def->data.tcp.reconnect);
>  
>          if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
>              goto error;
> @@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>              virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels,
>                                               def->seclabels, flags);
>  
> -            virDomainChrSourceReconnectDefFormat(&childBuf,
> -                                                 &def->data.nix.reconnect);
> +            if (!def->data.nix.listen)
> +                virDomainChrSourceReconnectDefFormat(&childBuf,
> +                                                     &def->data.nix.reconnect);
>  
>              if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
>                  goto error;
> 

This looks like a workaround. Because def->data.tcp.listen shouldn't be
set if reconnect is enabled and vice versa. And
virDomainChrSourceReconnectDefFormat() shortcuts out. Or you want the
following:

    <channel type='tcp'>
      <source mode='bind' host='localhost' service='5678'>
        <reconnect enabled='no'/>
      </source>
      <target type='virtio' name='test2'/>
    </channel>

to be turned into:

    <channel type='tcp'>
      <source mode='bind' host='localhost' service='5678'/>
      <target type='virtio' name='test2'/>
    </channel>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Posted by Pavel Hrdina 7 years, 8 months ago 
On Wed, Aug 30, 2017 at 02:32:50PM +0200, Michal Privoznik wrote:
> On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  src/conf/domain_conf.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index f7574d77b6..7f443e5b4d 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
> >              virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'",
> >                                def->data.tcp.tlsFromConfig);
> >  
> > -        virDomainChrSourceReconnectDefFormat(&childBuf,
> > -                                             &def->data.tcp.reconnect);
> > +        if (!def->data.tcp.listen)
> > +            virDomainChrSourceReconnectDefFormat(&childBuf,
> > +                                                 &def->data.tcp.reconnect);
> >  
> >          if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> >              goto error;
> > @@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
> >              virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels,
> >                                               def->seclabels, flags);
> >  
> > -            virDomainChrSourceReconnectDefFormat(&childBuf,
> > -                                                 &def->data.nix.reconnect);
> > +            if (!def->data.nix.listen)
> > +                virDomainChrSourceReconnectDefFormat(&childBuf,
> > +                                                     &def->data.nix.reconnect);
> >  
> >              if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> >                  goto error;
> > 
> 
> This looks like a workaround. Because def->data.tcp.listen shouldn't be
> set if reconnect is enabled and vice versa. And
> virDomainChrSourceReconnectDefFormat() shortcuts out. Or you want the
> following:
> 
>     <channel type='tcp'>
>       <source mode='bind' host='localhost' service='5678'>
>         <reconnect enabled='no'/>
>       </source>
>       <target type='virtio' name='test2'/>
>     </channel>
> 
> to be turned into:
> 
>     <channel type='tcp'>
>       <source mode='bind' host='localhost' service='5678'/>
>       <target type='virtio' name='test2'/>
>     </channel>
> 
> Michal

Yes, it's kind of workaround for the case where you will set

  <channel type='unix'>
    <source mode='connect' path='/var/lib/libvirt/qemu/channel/target/domain-test/test2'>
      <reconnect enabled='no'/>
    </source>
    <target type='virtio' name='test2'/>
  </channel>

Without this patch it would lead to:

  <channel type='unix'>
    <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-5-test/test2'>
      <reconnect enabled='no'/>
    </source>
    <target type='virtio' name='test2'/>
  </channel>

because we remove the auto-generated path and while starting a guest
we generate a new one and sets the mode to "bind".

This patch makes sure that in this case the XML of live guest is
correct.

The proper fix would be to update _virDomainChrSourceDef by changing
'bool listen' into 'virDomainChrSourceModeType mode' and the parse would
properly store three different values: default, connect, bind.  This
would require rewrite a lot of code which is not suitable for a freeze,
therefore this workaround.  I'm planning to fix it properly so that
workaround is not required anymore.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Posted by Michal Privoznik 7 years, 8 months ago 
On 08/30/2017 02:54 PM, Pavel Hrdina wrote:
> On Wed, Aug 30, 2017 at 02:32:50PM +0200, Michal Privoznik wrote:
>> On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>  src/conf/domain_conf.c | 10 ++++++----
>>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>>> index f7574d77b6..7f443e5b4d 100644
>>> --- a/src/conf/domain_conf.c
>>> +++ b/src/conf/domain_conf.c
>>> @@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>>>              virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'",
>>>                                def->data.tcp.tlsFromConfig);
>>>  
>>> -        virDomainChrSourceReconnectDefFormat(&childBuf,
>>> -                                             &def->data.tcp.reconnect);
>>> +        if (!def->data.tcp.listen)
>>> +            virDomainChrSourceReconnectDefFormat(&childBuf,
>>> +                                                 &def->data.tcp.reconnect);
>>>  
>>>          if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
>>>              goto error;
>>> @@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>>>              virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels,
>>>                                               def->seclabels, flags);
>>>  
>>> -            virDomainChrSourceReconnectDefFormat(&childBuf,
>>> -                                                 &def->data.nix.reconnect);
>>> +            if (!def->data.nix.listen)
>>> +                virDomainChrSourceReconnectDefFormat(&childBuf,
>>> +                                                     &def->data.nix.reconnect);
>>>  
>>>              if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
>>>                  goto error;
>>>
>>
>> This looks like a workaround. Because def->data.tcp.listen shouldn't be
>> set if reconnect is enabled and vice versa. And
>> virDomainChrSourceReconnectDefFormat() shortcuts out. Or you want the
>> following:
>>
>>     <channel type='tcp'>
>>       <source mode='bind' host='localhost' service='5678'>
>>         <reconnect enabled='no'/>
>>       </source>
>>       <target type='virtio' name='test2'/>
>>     </channel>
>>
>> to be turned into:
>>
>>     <channel type='tcp'>
>>       <source mode='bind' host='localhost' service='5678'/>
>>       <target type='virtio' name='test2'/>
>>     </channel>
>>
>> Michal
> 
> Yes, it's kind of workaround for the case where you will set
> 
>   <channel type='unix'>
>     <source mode='connect' path='/var/lib/libvirt/qemu/channel/target/domain-test/test2'>
>       <reconnect enabled='no'/>
>     </source>
>     <target type='virtio' name='test2'/>
>   </channel>
> 
> Without this patch it would lead to:
> 
>   <channel type='unix'>
>     <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-5-test/test2'>
>       <reconnect enabled='no'/>
>     </source>
>     <target type='virtio' name='test2'/>
>   </channel>
> 
> because we remove the auto-generated path and while starting a guest
> we generate a new one and sets the mode to "bind".
> 
> This patch makes sure that in this case the XML of live guest is
> correct.
> 
> The proper fix would be to update _virDomainChrSourceDef by changing
> 'bool listen' into 'virDomainChrSourceModeType mode' and the parse would
> properly store three different values: default, connect, bind.  This
> would require rewrite a lot of code which is not suitable for a freeze,
> therefore this workaround.  I'm planning to fix it properly so that
> workaround is not required anymore.

Exactly. So what are we worried more about? Pushing this temporal
workaround or having not nice looking, but still valid and sensible XML?
Technically, mode='bind' and reconnect='no' is a valid combination.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/5] conf: make sure that chardev reconnect is formatted only for connect mode
Posted by Pavel Hrdina 7 years, 8 months ago 
On Wed, Aug 30, 2017 at 03:38:13PM +0200, Michal Privoznik wrote:
> On 08/30/2017 02:54 PM, Pavel Hrdina wrote:
> > On Wed, Aug 30, 2017 at 02:32:50PM +0200, Michal Privoznik wrote:
> >> On 08/30/2017 01:40 PM, Pavel Hrdina wrote:
> >>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >>> ---
> >>>  src/conf/domain_conf.c | 10 ++++++----
> >>>  1 file changed, 6 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >>> index f7574d77b6..7f443e5b4d 100644
> >>> --- a/src/conf/domain_conf.c
> >>> +++ b/src/conf/domain_conf.c
> >>> @@ -23257,8 +23257,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
> >>>              virBufferAsprintf(&attrBuf, " tlsFromConfig='%d'",
> >>>                                def->data.tcp.tlsFromConfig);
> >>>  
> >>> -        virDomainChrSourceReconnectDefFormat(&childBuf,
> >>> -                                             &def->data.tcp.reconnect);
> >>> +        if (!def->data.tcp.listen)
> >>> +            virDomainChrSourceReconnectDefFormat(&childBuf,
> >>> +                                                 &def->data.tcp.reconnect);
> >>>  
> >>>          if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> >>>              goto error;
> >>> @@ -23276,8 +23277,9 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
> >>>              virDomainSourceDefFormatSeclabel(&childBuf, def->nseclabels,
> >>>                                               def->seclabels, flags);
> >>>  
> >>> -            virDomainChrSourceReconnectDefFormat(&childBuf,
> >>> -                                                 &def->data.nix.reconnect);
> >>> +            if (!def->data.nix.listen)
> >>> +                virDomainChrSourceReconnectDefFormat(&childBuf,
> >>> +                                                     &def->data.nix.reconnect);
> >>>  
> >>>              if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0)
> >>>                  goto error;
> >>>
> >>
> >> This looks like a workaround. Because def->data.tcp.listen shouldn't be
> >> set if reconnect is enabled and vice versa. And
> >> virDomainChrSourceReconnectDefFormat() shortcuts out. Or you want the
> >> following:
> >>
> >>     <channel type='tcp'>
> >>       <source mode='bind' host='localhost' service='5678'>
> >>         <reconnect enabled='no'/>
> >>       </source>
> >>       <target type='virtio' name='test2'/>
> >>     </channel>
> >>
> >> to be turned into:
> >>
> >>     <channel type='tcp'>
> >>       <source mode='bind' host='localhost' service='5678'/>
> >>       <target type='virtio' name='test2'/>
> >>     </channel>
> >>
> >> Michal
> > 
> > Yes, it's kind of workaround for the case where you will set
> > 
> >   <channel type='unix'>
> >     <source mode='connect' path='/var/lib/libvirt/qemu/channel/target/domain-test/test2'>
> >       <reconnect enabled='no'/>
> >     </source>
> >     <target type='virtio' name='test2'/>
> >   </channel>
> > 
> > Without this patch it would lead to:
> > 
> >   <channel type='unix'>
> >     <source mode='bind' path='/var/lib/libvirt/qemu/channel/target/domain-5-test/test2'>
> >       <reconnect enabled='no'/>
> >     </source>
> >     <target type='virtio' name='test2'/>
> >   </channel>
> > 
> > because we remove the auto-generated path and while starting a guest
> > we generate a new one and sets the mode to "bind".
> > 
> > This patch makes sure that in this case the XML of live guest is
> > correct.
> > 
> > The proper fix would be to update _virDomainChrSourceDef by changing
> > 'bool listen' into 'virDomainChrSourceModeType mode' and the parse would
> > properly store three different values: default, connect, bind.  This
> > would require rewrite a lot of code which is not suitable for a freeze,
> > therefore this workaround.  I'm planning to fix it properly so that
> > workaround is not required anymore.
> 
> Exactly. So what are we worried more about? Pushing this temporal
> workaround or having not nice looking, but still valid and sensible XML?
> Technically, mode='bind' and reconnect='no' is a valid combination.

The thing is that this also formats

  <reconnect enabled='yes' timeout='10'/>

for "bind" mode which is wrong and needs to be fixed.

BTW: this is same workaround as for the qemu command line format code.

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

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.