Re: [Xen-devel] [libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen

2016-02-24 Thread Joao Martins 


On 02/24/2016 04:31 AM, Jim Fehlig wrote:
> On 02/22/2016 04:15 PM, Joao Martins wrote:
>>
>> On 12/08/2015 04:06 PM, Jim Fehlig wrote:
>>> Daniel P. Berrange wrote:
 On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
> On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
>> On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
>>> In commit d2e5538b1, the libxl driver was changed to copy interface
>>> names autogenerated by libxl to the corresponding network def in the
>>> domain's virDomainDef object. The copied name is freed when the domain
>>> transitions to the shutoff state. But when migrating a domain, the
>>> autogenerated name is included in the XML sent to the destination host.
>>> It is possible an interface with the same name already exists on the
>>> destination host, causing migration to fail. Indeed the Xen project's
>>> OSSTEST CI already encountered such a failure.
>>>
>>> This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing
>>> the autogenerated names to be excluded when parsing and formatting
>>> inactive config.
>>>
>>> Signed-off-by: Jim Fehlig 
>>> ---
>>>
>>> This is an alternative approach to Joao's fix for this regression
>>>
>>> https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
>>>
>>> I think it is the same approach used by the qemu driver. My only
>>> reservation is that it expands the potential for clashes with
>>> user-defined names. I.e. with this change both 'vnet' and 'vif' are
>>> reserved prefixes.
>> Hmm, yes, tricky one.
>>
>> If we only care about XML parsing, then you could register a post
>> parse callback instead to do this.
> AFAIK, XML parsing is all that's in play here.
>
>> I'm not clear why we also have it in the virDomainNetDefFormat
>> method - and we can't solve that with a post-parse callback.
>>
>>
>> The other option would be to make the reserved prefix be a
>> capability that the parser/formatter could read.
> This seems like the best option, since a post-parse callback doesn't 
> solve the
> problem in virDomainNetDefFormat. It also has the upshot of making the 
> prefix
> visible and known to users. But I doubt such a change is suitable during 
> 1.3.0
> freeze.  With the freeze in mind, seems the best solution to the libxl 
> migration
> regression is revert d2e5538b1. It can be added again post-1.3.0 release, 
> after
> adding the prefix to capabilities.
>
> DV, since you may be making the release soon, feel free to revert 
> d2e5538b1 if
> you agree.
 Yeah, just go ahead & revert it Jim,  DV isn't doing the releae until
 tomorrow morning
>>> I've pushed the revert.
>>>
>>> Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after 
>>> exposing
>>> the prefix in capabilities.
>>>
>> Hey Jim,
>>
>> I had the impression we had pushed this back in (i.e. cherry-picking d2e5538)
>> but I was double checking and that's doesn't seem to be case. In adding 
>> support
>> for the prefix in capabilities I found out one issue on the migration failure
>> path leading to dereferencing a NULL pointer on the destination. If you agree
>> could we squash the following chunk in addition to the cherry-pick of 
>> d2e5538:
>>
>> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
>> index 63c5b24..f73bfb3 100644
>> --- a/src/libxl/libxl_domain.c
>> +++ b/src/libxl/libxl_domain.c
>> @@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>>  for (i = 0; i < vm->def->nnets; i++) {
>>  virDomainNetDefPtr net = vm->def->nets[i];
>>
>> -if (STRPREFIX(net->ifname, "vif"))
>> +if (net->ifname && STRPREFIX(net->ifname, "vif"))
>>  VIR_FREE(net->ifname);
>>  }
>>  }
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 3f4457f..5479441 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
>>  .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */
>>  .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
>>  .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
>> -.domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
>> +.domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */
>>  .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 
>> */
>>  .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 
>> 0.9.0 */
>>  .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */
>>
>>
>> Or do you think it should be deferred to 1.3.3 ?
> 
> I wanted to ask you about this on the recent Xen+libvirt+OpenStack sync. 
> Thanks
> for checking. It would be a shame if this patch didn't make

Re: [Xen-devel] [libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen

2016-02-22 Thread Joao Martins 


On 12/08/2015 04:06 PM, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
>> On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
>>> On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
 On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
> In commit d2e5538b1, the libxl driver was changed to copy interface
> names autogenerated by libxl to the corresponding network def in the
> domain's virDomainDef object. The copied name is freed when the domain
> transitions to the shutoff state. But when migrating a domain, the
> autogenerated name is included in the XML sent to the destination host.
> It is possible an interface with the same name already exists on the
> destination host, causing migration to fail. Indeed the Xen project's
> OSSTEST CI already encountered such a failure.
>
> This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing
> the autogenerated names to be excluded when parsing and formatting
> inactive config.
>
> Signed-off-by: Jim Fehlig 
> ---
>
> This is an alternative approach to Joao's fix for this regression
>
> https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
>
> I think it is the same approach used by the qemu driver. My only
> reservation is that it expands the potential for clashes with
> user-defined names. I.e. with this change both 'vnet' and 'vif' are
> reserved prefixes.
 Hmm, yes, tricky one.

 If we only care about XML parsing, then you could register a post
 parse callback instead to do this.
>>> AFAIK, XML parsing is all that's in play here.
>>>
 I'm not clear why we also have it in the virDomainNetDefFormat
 method - and we can't solve that with a post-parse callback.


 The other option would be to make the reserved prefix be a
 capability that the parser/formatter could read.
>>> This seems like the best option, since a post-parse callback doesn't solve 
>>> the
>>> problem in virDomainNetDefFormat. It also has the upshot of making the 
>>> prefix
>>> visible and known to users. But I doubt such a change is suitable during 
>>> 1.3.0
>>> freeze.  With the freeze in mind, seems the best solution to the libxl 
>>> migration
>>> regression is revert d2e5538b1. It can be added again post-1.3.0 release, 
>>> after
>>> adding the prefix to capabilities.
>>>
>>> DV, since you may be making the release soon, feel free to revert d2e5538b1 
>>> if
>>> you agree.
>>
>> Yeah, just go ahead & revert it Jim,  DV isn't doing the releae until
>> tomorrow morning
> 
> I've pushed the revert.
> 
> Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing
> the prefix in capabilities.
> 
Hey Jim,

I had the impression we had pushed this back in (i.e. cherry-picking d2e5538)
but I was double checking and that's doesn't seem to be case. In adding support
for the prefix in capabilities I found out one issue on the migration failure
path leading to dereferencing a NULL pointer on the destination. If you agree
could we squash the following chunk in addition to the cherry-pick of d2e5538:

diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
index 63c5b24..f73bfb3 100644
--- a/src/libxl/libxl_domain.c
+++ b/src/libxl/libxl_domain.c
@@ -768,7 +768,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
 for (i = 0; i < vm->def->nnets; i++) {
 virDomainNetDefPtr net = vm->def->nets[i];

-if (STRPREFIX(net->ifname, "vif"))
+if (net->ifname && STRPREFIX(net->ifname, "vif"))
 VIR_FREE(net->ifname);
 }
 }
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3f4457f..5479441 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -5590,7 +5590,7 @@ static virHypervisorDriver libxlHypervisorDriver = {
 .domainGetJobStats = libxlDomainGetJobStats, /* 1.3.1 */
 .domainMemoryStats = libxlDomainMemoryStats, /* 1.3.0 */
 .domainGetCPUStats = libxlDomainGetCPUStats, /* 1.3.0 */
-.domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.0 */
+.domainInterfaceStats = libxlDomainInterfaceStats, /* 1.3.2 */
 .connectDomainEventRegister = libxlConnectDomainEventRegister, /* 0.9.0 */
 .connectDomainEventDeregister = libxlConnectDomainEventDeregister, /* 
0.9.0 */
 .domainManagedSave = libxlDomainManagedSave, /* 0.9.2 */


Or do you think it should be deferred to 1.3.3 ?

Thanks,
Joao

> Regards,
> Jim
> 
> --
> libvir-list mailing list
> libvir-l...@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen

2015-12-08 Thread Jim Fehlig 
Daniel P. Berrange wrote:
> On Mon, Dec 07, 2015 at 10:59:32PM -0700, Jim Fehlig wrote:
>> On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
>>> On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
 In commit d2e5538b1, the libxl driver was changed to copy interface
 names autogenerated by libxl to the corresponding network def in the
 domain's virDomainDef object. The copied name is freed when the domain
 transitions to the shutoff state. But when migrating a domain, the
 autogenerated name is included in the XML sent to the destination host.
 It is possible an interface with the same name already exists on the
 destination host, causing migration to fail. Indeed the Xen project's
 OSSTEST CI already encountered such a failure.

 This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing
 the autogenerated names to be excluded when parsing and formatting
 inactive config.

 Signed-off-by: Jim Fehlig 
 ---

 This is an alternative approach to Joao's fix for this regression

 https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html

 I think it is the same approach used by the qemu driver. My only
 reservation is that it expands the potential for clashes with
 user-defined names. I.e. with this change both 'vnet' and 'vif' are
 reserved prefixes.
>>> Hmm, yes, tricky one.
>>>
>>> If we only care about XML parsing, then you could register a post
>>> parse callback instead to do this.
>> AFAIK, XML parsing is all that's in play here.
>>
>>> I'm not clear why we also have it in the virDomainNetDefFormat
>>> method - and we can't solve that with a post-parse callback.
>>>
>>>
>>> The other option would be to make the reserved prefix be a
>>> capability that the parser/formatter could read.
>> This seems like the best option, since a post-parse callback doesn't solve 
>> the
>> problem in virDomainNetDefFormat. It also has the upshot of making the prefix
>> visible and known to users. But I doubt such a change is suitable during 
>> 1.3.0
>> freeze.  With the freeze in mind, seems the best solution to the libxl 
>> migration
>> regression is revert d2e5538b1. It can be added again post-1.3.0 release, 
>> after
>> adding the prefix to capabilities.
>>
>> DV, since you may be making the release soon, feel free to revert d2e5538b1 
>> if
>> you agree.
> 
> Yeah, just go ahead & revert it Jim,  DV isn't doing the releae until
> tomorrow morning

I've pushed the revert.

Joao, sorry for yanking this for 1.3.0. We can get it in 1.3.1, after exposing
the prefix in capabilities.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen

2015-12-07 Thread Jim Fehlig 
On 12/07/2015 11:52 AM, Daniel P. Berrange wrote:
> On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
>> In commit d2e5538b1, the libxl driver was changed to copy interface
>> names autogenerated by libxl to the corresponding network def in the
>> domain's virDomainDef object. The copied name is freed when the domain
>> transitions to the shutoff state. But when migrating a domain, the
>> autogenerated name is included in the XML sent to the destination host.
>> It is possible an interface with the same name already exists on the
>> destination host, causing migration to fail. Indeed the Xen project's
>> OSSTEST CI already encountered such a failure.
>>
>> This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing
>> the autogenerated names to be excluded when parsing and formatting
>> inactive config.
>>
>> Signed-off-by: Jim Fehlig 
>> ---
>>
>> This is an alternative approach to Joao's fix for this regression
>>
>> https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
>>
>> I think it is the same approach used by the qemu driver. My only
>> reservation is that it expands the potential for clashes with
>> user-defined names. I.e. with this change both 'vnet' and 'vif' are
>> reserved prefixes.
> Hmm, yes, tricky one.
>
> If we only care about XML parsing, then you could register a post
> parse callback instead to do this.

AFAIK, XML parsing is all that's in play here.

> I'm not clear why we also have it in the virDomainNetDefFormat
> method - and we can't solve that with a post-parse callback.
>
>
> The other option would be to make the reserved prefix be a
> capability that the parser/formatter could read.

This seems like the best option, since a post-parse callback doesn't solve the
problem in virDomainNetDefFormat. It also has the upshot of making the prefix
visible and known to users. But I doubt such a change is suitable during 1.3.0
freeze.  With the freeze in mind, seems the best solution to the libxl migration
regression is revert d2e5538b1. It can be added again post-1.3.0 release, after
adding the prefix to capabilities.

DV, since you may be making the release soon, feel free to revert d2e5538b1 if
you agree.

Regards,
Jim


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [libvirt] [FOR 1.3.0 PATCH] conf: add net device prefix for Xen

2015-12-07 Thread Daniel P. Berrange 
On Mon, Dec 07, 2015 at 09:42:21AM -0700, Jim Fehlig wrote:
> In commit d2e5538b1, the libxl driver was changed to copy interface
> names autogenerated by libxl to the corresponding network def in the
> domain's virDomainDef object. The copied name is freed when the domain
> transitions to the shutoff state. But when migrating a domain, the
> autogenerated name is included in the XML sent to the destination host.
> It is possible an interface with the same name already exists on the
> destination host, causing migration to fail. Indeed the Xen project's
> OSSTEST CI already encountered such a failure.
> 
> This patch defines another VIR_NET_GENERATED_PREFIX for Xen, allowing
> the autogenerated names to be excluded when parsing and formatting
> inactive config.
> 
> Signed-off-by: Jim Fehlig 
> ---
> 
> This is an alternative approach to Joao's fix for this regression
> 
> https://www.redhat.com/archives/libvir-list/2015-December/msg00197.html
> 
> I think it is the same approach used by the qemu driver. My only
> reservation is that it expands the potential for clashes with
> user-defined names. I.e. with this change both 'vnet' and 'vif' are
> reserved prefixes.

Hmm, yes, tricky one.

If we only care about XML parsing, then you could register a post
parse callback instead to do this.

I'm not clear why we also have it in the virDomainNetDefFormat
method - and we can't solve that with a post-parse callback.


The other option would be to make the reserved prefix be a
capability that the parser/formatter could read.

> 
>  src/conf/domain_conf.c   | 6 --
>  src/conf/domain_conf.h   | 4 
>  src/libxl/libxl_domain.c | 5 +++--
>  3 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2f5c0ed..debcf4e 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8428,7 +8428,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>  ifname = virXMLPropString(cur, "dev");
>  if (ifname &&
>  (flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
> -STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX)) {
> +(STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX) ||
> + STRPREFIX(ifname, VIR_NET_GENERATED_PREFIX_XEN))) {
>  /* An auto-generated target name, blank it out */
>  VIR_FREE(ifname);
>  }
> @@ -19790,7 +19791,8 @@ virDomainNetDefFormat(virBufferPtr buf,
>  virBufferEscapeString(buf, "\n", 
> def->domain_name);
>  if (def->ifname &&
>  !((flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE) &&
> -  (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX {
> +  (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX) ||
> +   STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX_XEN {
>  /* Skip auto-generated target names for inactive config. */
>  virBufferEscapeString(buf, "\n", def->ifname);
>  }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 90d8e13..d2cc26f 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1085,6 +1085,10 @@ struct _virDomainNetDef {
>   * by libvirt, and cannot be used for a persistent network name.  */
>  # define VIR_NET_GENERATED_PREFIX "vnet"
>  
> +/* Used for prefix of ifname of any network name generated dynamically
> + * by libvirt for Xen, and cannot be used for a persistent network name.  */
> +# define VIR_NET_GENERATED_PREFIX_XEN "vif"
> +
>  typedef enum {
>  VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT = 0,
>  VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED,
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index ef92974..c5d84a4 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -734,7 +734,7 @@ libxlDomainCleanup(libxlDriverPrivatePtr driver,
>  for (i = 0; i < vm->def->nnets; i++) {
>  virDomainNetDefPtr net = vm->def->nets[i];
>  
> -if (STRPREFIX(net->ifname, "vif"))
> +if (STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX_XEN))
>  VIR_FREE(net->ifname);
>  }
>  }
> @@ -918,7 +918,8 @@ libxlDomainCreateIfaceNames(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>  if (net->ifname)
>  continue;
>  
> -ignore_value(virAsprintf(>ifname, "vif%d.%d%s",
> +ignore_value(virAsprintf(>ifname,
> + VIR_NET_GENERATED_PREFIX_XEN "%d.%d%s",
>   def->id, x_nic->devid, suffix));
>  }
>  }
> -- 
> 2.6.1
> 
> --
> libvir-list mailing list
> libvir-l...@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o-