Re: [libvirt] [PATCH 3/3] domain: fix migration to older libvirt

[Pavel Hrdina]

On Mon, Oct 24, 2016 at 10:31:42AM -0400, John Ferlan wrote:
> 
> 
> On 10/24/2016 10:28 AM, Pavel Hrdina wrote:
> > On Mon, Oct 24, 2016 at 10:11:53AM -0400, John Ferlan wrote:
> >>
> >>
> >> On 10/24/2016 08:41 AM, Pavel Hrdina wrote:
> >>> Since TLS feature was introduced in libvirt 2.3.0 we have to modify
> >>
> >> Since TLS was introduced hostwide for libvirt 2.3.0 and a domain
> >> configurable haveTLS was implemented for libvirt 2.4.0, we have to
> >> modify the
> >>
> >>> migratable XML for specific case where 'tls' attribute is based on
> >>
> >> s/where/where the/
> >>
> >>> setting from qemu.conf.
> >>>
> >>> Signed-off-by: Pavel Hrdina 
> >>> ---
> >>>  src/conf/domain_conf.c | 24 +++-
> >>>  src/conf/domain_conf.h |  1 +
> >>>  src/qemu/qemu_domain.c |  1 +
> >>>  3 files changed, 25 insertions(+), 1 deletion(-)
> >>>
> >>
> >>
> >> TBH this is ultimately very odd/confusing to read.  The "tlsFromConfig"
> >> doesn't end up in the config XML right?  Just the migratable.
> > 
> > The "tlsFromConfig" is libvirt internal attribute and is stored only in
> > status XML to ensure that when libvirtd is restarted this internal flag is
> > not lost by the restart.
> > 
> > That flag is used to decide whether we should put *tls* attribute to
> > migratable XML or not.  I'll add it to the commit message to make it clear.
> > 
> > I'll wait with pushing for your reply.
> 
> That's fine -

Thanks, pushed now.

Pavel


signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 3/3] domain: fix migration to older libvirt

[John Ferlan]

On 10/24/2016 10:28 AM, Pavel Hrdina wrote:
> On Mon, Oct 24, 2016 at 10:11:53AM -0400, John Ferlan wrote:
>>
>>
>> On 10/24/2016 08:41 AM, Pavel Hrdina wrote:
>>> Since TLS feature was introduced in libvirt 2.3.0 we have to modify
>>
>> Since TLS was introduced hostwide for libvirt 2.3.0 and a domain
>> configurable haveTLS was implemented for libvirt 2.4.0, we have to
>> modify the
>>
>>> migratable XML for specific case where 'tls' attribute is based on
>>
>> s/where/where the/
>>
>>> setting from qemu.conf.
>>>
>>> Signed-off-by: Pavel Hrdina 
>>> ---
>>>  src/conf/domain_conf.c | 24 +++-
>>>  src/conf/domain_conf.h |  1 +
>>>  src/qemu/qemu_domain.c |  1 +
>>>  3 files changed, 25 insertions(+), 1 deletion(-)
>>>
>>
>>
>> TBH this is ultimately very odd/confusing to read.  The "tlsFromConfig"
>> doesn't end up in the config XML right?  Just the migratable.
> 
> The "tlsFromConfig" is libvirt internal attribute and is stored only in
> status XML to ensure that when libvirtd is restarted this internal flag is
> not lost by the restart.
> 
> That flag is used to decide whether we should put *tls* attribute to
> migratable XML or not.  I'll add it to the commit message to make it clear.
> 
> I'll wait with pushing for your reply.

That's fine -

John

> 
> Pavel
> 
>> ACK since you've shown in the previous series that the permutations
>> work. Perhaps an extra word or two in the commit message regarding the
>> flag and how it's used will be helpful for the person who reads the code
>> once we've both (shortly) forgotten about it
>>
>> John
>>
[...]

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

Re: [libvirt] [PATCH 3/3] domain: fix migration to older libvirt

[Pavel Hrdina]

On Mon, Oct 24, 2016 at 10:11:53AM -0400, John Ferlan wrote:
> 
> 
> On 10/24/2016 08:41 AM, Pavel Hrdina wrote:
> > Since TLS feature was introduced in libvirt 2.3.0 we have to modify
> 
> Since TLS was introduced hostwide for libvirt 2.3.0 and a domain
> configurable haveTLS was implemented for libvirt 2.4.0, we have to
> modify the
> 
> > migratable XML for specific case where 'tls' attribute is based on
> 
> s/where/where the/
> 
> > setting from qemu.conf.
> > 
> > Signed-off-by: Pavel Hrdina 
> > ---
> >  src/conf/domain_conf.c | 24 +++-
> >  src/conf/domain_conf.h |  1 +
> >  src/qemu/qemu_domain.c |  1 +
> >  3 files changed, 25 insertions(+), 1 deletion(-)
> > 
> 
> 
> TBH this is ultimately very odd/confusing to read.  The "tlsFromConfig"
> doesn't end up in the config XML right?  Just the migratable.

The "tlsFromConfig" is libvirt internal attribute and is stored only in
status XML to ensure that when libvirtd is restarted this internal flag is
not lost by the restart.

That flag is used to decide whether we should put *tls* attribute to
migratable XML or not.  I'll add it to the commit message to make it clear.

I'll wait with pushing for your reply.

Pavel

> ACK since you've shown in the previous series that the permutations
> work. Perhaps an extra word or two in the commit message regarding the
> flag and how it's used will be helpful for the person who reads the code
> once we've both (shortly) forgotten about it
> 
> John
> 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 6e814b3..f556e4c 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -1999,6 +1999,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr 
> > dest,
> >  return -1;
> >  
> >  dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
> > +dest->data.tcp.tlsFromConfig = src->data.tcp.tlsFromConfig;
> >  break;
> >  
> >  case VIR_DOMAIN_CHR_TYPE_UNIX:
> > @@ -10042,6 +10043,7 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >  char *slave = NULL;
> >  char *append = NULL;
> >  char *haveTLS = NULL;
> > +char *tlsFromConfig = NULL;
> >  int remaining = 0;
> >  
> >  while (cur != NULL) {
> > @@ -10051,6 +10053,8 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >  mode = virXMLPropString(cur, "mode");
> >  if (!haveTLS)
> >  haveTLS = virXMLPropString(cur, "tls");
> > +if (!tlsFromConfig)
> > +tlsFromConfig = virXMLPropString(cur, "tlsFromConfig");
> >  
> >  switch ((virDomainChrType) def->type) {
> >  case VIR_DOMAIN_CHR_TYPE_FILE:
> > @@ -10236,6 +10240,18 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >  goto error;
> >  }
> >  
> > +if (tlsFromConfig &&
> > +flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
> > +int tmp;
> > +if (virStrToLong_i(tlsFromConfig, NULL, 10, ) < 0) {
> > +virReportError(VIR_ERR_XML_ERROR,
> > +   _("Invalid tlsFromConfig value: %s"),
> > +   tlsFromConfig);
> > +goto error;
> > +}
> > +def->data.tcp.tlsFromConfig = !!tmp;
> > +}
> > +
> >  if (!protocol)
> >  def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
> >  else if ((def->data.tcp.protocol =
> > @@ -10321,6 +10337,7 @@ 
> > virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
> >  VIR_FREE(logappend);
> >  VIR_FREE(logfile);
> >  VIR_FREE(haveTLS);
> > +VIR_FREE(tlsFromConfig);
> >  
> >  return remaining;
> >  
> > @@ -21508,9 +21525,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
> >def->data.tcp.listen ? "bind" : "connect");
> >  virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host);
> >  virBufferEscapeString(buf, "service='%s'", def->data.tcp.service);
> > -if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT)
> > +if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT &&
> > +!(flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
> > +  def->data.tcp.tlsFromConfig))
> >  virBufferAsprintf(buf, " tls='%s'",
> >  virTristateBoolTypeToString(def->data.tcp.haveTLS));
> > +if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
> > +virBufferAsprintf(buf, " tlsFromConfig='%d'",
> > +  def->data.tcp.tlsFromConfig);
> >  virBufferAddLit(buf, "/>\n");
> >  
> >  virBufferAsprintf(buf, "\n",
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index f1da9c3..dff28c0 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -1096,6 +1096,7 @@ struct _virDomainChrSourceDef 

Re: [libvirt] [PATCH 3/3] domain: fix migration to older libvirt

[John Ferlan]

On 10/24/2016 08:41 AM, Pavel Hrdina wrote:
> Since TLS feature was introduced in libvirt 2.3.0 we have to modify

Since TLS was introduced hostwide for libvirt 2.3.0 and a domain
configurable haveTLS was implemented for libvirt 2.4.0, we have to
modify the

> migratable XML for specific case where 'tls' attribute is based on

s/where/where the/

> setting from qemu.conf.
> 
> Signed-off-by: Pavel Hrdina 
> ---
>  src/conf/domain_conf.c | 24 +++-
>  src/conf/domain_conf.h |  1 +
>  src/qemu/qemu_domain.c |  1 +
>  3 files changed, 25 insertions(+), 1 deletion(-)
> 


TBH this is ultimately very odd/confusing to read.  The "tlsFromConfig"
doesn't end up in the config XML right?  Just the migratable.

ACK since you've shown in the previous series that the permutations
work. Perhaps an extra word or two in the commit message regarding the
flag and how it's used will be helpful for the person who reads the code
once we've both (shortly) forgotten about it

John

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6e814b3..f556e4c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1999,6 +1999,7 @@ virDomainChrSourceDefCopy(virDomainChrSourceDefPtr dest,
>  return -1;
>  
>  dest->data.tcp.haveTLS = src->data.tcp.haveTLS;
> +dest->data.tcp.tlsFromConfig = src->data.tcp.tlsFromConfig;
>  break;
>  
>  case VIR_DOMAIN_CHR_TYPE_UNIX:
> @@ -10042,6 +10043,7 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  char *slave = NULL;
>  char *append = NULL;
>  char *haveTLS = NULL;
> +char *tlsFromConfig = NULL;
>  int remaining = 0;
>  
>  while (cur != NULL) {
> @@ -10051,6 +10053,8 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  mode = virXMLPropString(cur, "mode");
>  if (!haveTLS)
>  haveTLS = virXMLPropString(cur, "tls");
> +if (!tlsFromConfig)
> +tlsFromConfig = virXMLPropString(cur, "tlsFromConfig");
>  
>  switch ((virDomainChrType) def->type) {
>  case VIR_DOMAIN_CHR_TYPE_FILE:
> @@ -10236,6 +10240,18 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  goto error;
>  }
>  
> +if (tlsFromConfig &&
> +flags & VIR_DOMAIN_DEF_PARSE_STATUS) {
> +int tmp;
> +if (virStrToLong_i(tlsFromConfig, NULL, 10, ) < 0) {
> +virReportError(VIR_ERR_XML_ERROR,
> +   _("Invalid tlsFromConfig value: %s"),
> +   tlsFromConfig);
> +goto error;
> +}
> +def->data.tcp.tlsFromConfig = !!tmp;
> +}
> +
>  if (!protocol)
>  def->data.tcp.protocol = VIR_DOMAIN_CHR_TCP_PROTOCOL_RAW;
>  else if ((def->data.tcp.protocol =
> @@ -10321,6 +10337,7 @@ 
> virDomainChrSourceDefParseXML(virDomainChrSourceDefPtr def,
>  VIR_FREE(logappend);
>  VIR_FREE(logfile);
>  VIR_FREE(haveTLS);
> +VIR_FREE(tlsFromConfig);
>  
>  return remaining;
>  
> @@ -21508,9 +21525,14 @@ virDomainChrSourceDefFormat(virBufferPtr buf,
>def->data.tcp.listen ? "bind" : "connect");
>  virBufferEscapeString(buf, "host='%s' ", def->data.tcp.host);
>  virBufferEscapeString(buf, "service='%s'", def->data.tcp.service);
> -if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT)
> +if (def->data.tcp.haveTLS != VIR_TRISTATE_BOOL_ABSENT &&
> +!(flags & VIR_DOMAIN_DEF_FORMAT_MIGRATABLE &&
> +  def->data.tcp.tlsFromConfig))
>  virBufferAsprintf(buf, " tls='%s'",
>  virTristateBoolTypeToString(def->data.tcp.haveTLS));
> +if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS)
> +virBufferAsprintf(buf, " tlsFromConfig='%d'",
> +  def->data.tcp.tlsFromConfig);
>  virBufferAddLit(buf, "/>\n");
>  
>  virBufferAsprintf(buf, "\n",
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index f1da9c3..dff28c0 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1096,6 +1096,7 @@ struct _virDomainChrSourceDef {
>  int protocol;
>  bool tlscreds;
>  int haveTLS; /* enum virTristateBool */
> +bool tlsFromConfig;
>  } tcp;
>  struct {
>  char *bindHost;
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 6c0..41ac52d 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -6204,6 +6204,7 @@ 
> qemuDomainPrepareChardevSourceTLS(virDomainChrSourceDefPtr source,
>  source->data.tcp.haveTLS = VIR_TRISTATE_BOOL_YES;
>  else
>  source->data.tcp.haveTLS = VIR_TRISTATE_BOOL_NO;
> +