Xen devel - Aug 2012 - [PATCH] libxl: fix usage of backend parameter and run_hotplug_scripts

vif interfaces allows the user to specify the domain that should run
the backend (also known as driver domain) using the ''backend''
parameter. This is not compatible with run_hotplug_scripts=1, since
libxl can only run the hotplug scripts from the Domain 0.

Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
---
 docs/misc/xl-network-configuration.markdown |    6 ++++--
 tools/libxl/libxl.c                         |   14 ++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xl-network-configuration.markdown
b/docs/misc/xl-network-configuration.markdown
index 650926c..5e2f049 100644
--- a/docs/misc/xl-network-configuration.markdown
+++ b/docs/misc/xl-network-configuration.markdown
@@ -122,8 +122,10 @@ specified IP address to be used by the guest (blocking all
others).
 ### backend
 
 Specifies the backend domain which this device should attach to. This
-defaults to domain 0. Specifying another domain requires setting up a
-driver domain which is outside the scope of this document.
+defaults to domain 0. This option does not work if `run_hotplug_scripts`
+is not disabled in xl.conf (see xl.conf(5) man page for more information
+on this option). Specifying another domain requires setting up a driver
+domain which is outside the scope of this document.
 
 ### rate
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 8ea3478..6b85cdc 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2474,6 +2474,8 @@ out:
 int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
                                  uint32_t domid)
 {
+    int run_hotplug_scripts;
+
     if (!nic->mtu)
         nic->mtu = 1492;
     if (!nic->model) {
@@ -2503,6 +2505,18 @@ int libxl__device_nic_setdefault(libxl__gc *gc,
libxl_device_nic *nic,
                                   libxl__xen_script_dir_path()) < 0 )
         return ERROR_FAIL;
 
+    run_hotplug_scripts = libxl__hotplug_settings(gc, XBT_NULL);
+    if (run_hotplug_scripts < 0) {
+        LOG(ERROR, "unable to get current hotplug scripts execution
setting");
+        return run_hotplug_scripts;
+    }
+    if (nic->backend_domid != LIBXL_TOOLSTACK_DOMID &&
run_hotplug_scripts) {
+        LOG(ERROR, "the vif ''backend='' option cannot be
used in conjunction "
+                   "with run_hotplug_scripts, please set
run_hotplug_scripts "
+                   "to 0 in xl.conf");
+        return ERROR_FAIL;
+    }
+
     switch (libxl__domain_type(gc, domid)) {
     case LIBXL_DOMAIN_TYPE_HVM:
         if (!nic->nictype)
-- 
1.7.7.5 (Apple Git-26)

On Tue, 2012-08-14 at 19:15 +0100, Roger Pau Monne
wrote:
> vif interfaces allows the user to specify the domain that should run
> the backend (also known as driver domain) using the
''backend''
> parameter. This is not compatible with run_hotplug_scripts=1, since
> libxl can only run the hotplug scripts from the Domain 0.
> 
> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
> ---
>  docs/misc/xl-network-configuration.markdown |    6 ++++--
>  tools/libxl/libxl.c                         |   14 ++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/misc/xl-network-configuration.markdown
b/docs/misc/xl-network-configuration.markdown
> index 650926c..5e2f049 100644
> --- a/docs/misc/xl-network-configuration.markdown
> +++ b/docs/misc/xl-network-configuration.markdown
> @@ -122,8 +122,10 @@ specified IP address to be used by the guest (blocking
all others).
>  ### backend
>  
>  Specifies the backend domain which this device should attach to. This
> -defaults to domain 0. Specifying another domain requires setting up a
> -driver domain which is outside the scope of this document.
> +defaults to domain 0. This option does not work if `run_hotplug_scripts`
> +is not disabled in xl.conf (see xl.conf(5) man page for more information
> +on this option). Specifying another domain requires setting up a driver
> +domain which is outside the scope of this document.
>  
>  ### rate
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 8ea3478..6b85cdc 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -2474,6 +2474,8 @@ out:
>  int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
>                                   uint32_t domid)
>  {
> +    int run_hotplug_scripts;
> +
>      if (!nic->mtu)
>          nic->mtu = 1492;
>      if (!nic->model) {
> @@ -2503,6 +2505,18 @@ int libxl__device_nic_setdefault(libxl__gc *gc,
libxl_device_nic *nic,
>                                    libxl__xen_script_dir_path()) < 0 )
>          return ERROR_FAIL;
>  
> +    run_hotplug_scripts = libxl__hotplug_settings(gc, XBT_NULL);
> +    if (run_hotplug_scripts < 0) {
> +        LOG(ERROR, "unable to get current hotplug scripts execution
setting");
Include the error value?
> +        return run_hotplug_scripts;
> +    }
> +    if (nic->backend_domid != LIBXL_TOOLSTACK_DOMID &&
run_hotplug_scripts) {
> +        LOG(ERROR, "the vif ''backend='' option
cannot be used in conjunction "
> +                   "with run_hotplug_scripts, please set
run_hotplug_scripts "
> +                   "to 0 in xl.conf");
This mention of xl.conf in libxl is a layering violation.

I think it would be fine for libxl to log something generic about
hotplug scripts and return an error and to have a more specific check t
parse time in xl which errors out with a reference to the config option.
> +        return ERROR_FAIL;
> +    }
> +
>      switch (libxl__domain_type(gc, domid)) {
>      case LIBXL_DOMAIN_TYPE_HVM:
>          if (!nic->nictype)

Ian Campbell wrote:
> On Tue, 2012-08-14 at 19:15 +0100, Roger Pau Monne wrote:
>> vif interfaces allows the user to specify the domain that should run
>> the backend (also known as driver domain) using the
''backend''
>> parameter. This is not compatible with run_hotplug_scripts=1, since
>> libxl can only run the hotplug scripts from the Domain 0.
>>
>> Signed-off-by: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>>  docs/misc/xl-network-configuration.markdown |    6 ++++--
>>  tools/libxl/libxl.c                         |   14 ++++++++++++++
>>  2 files changed, 18 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/misc/xl-network-configuration.markdown
b/docs/misc/xl-network-configuration.markdown
>> index 650926c..5e2f049 100644
>> --- a/docs/misc/xl-network-configuration.markdown
>> +++ b/docs/misc/xl-network-configuration.markdown
>> @@ -122,8 +122,10 @@ specified IP address to be used by the guest
(blocking all others).
>>  ### backend
>>  
>>  Specifies the backend domain which this device should attach to. This
>> -defaults to domain 0. Specifying another domain requires setting up a
>> -driver domain which is outside the scope of this document.
>> +defaults to domain 0. This option does not work if
`run_hotplug_scripts`
>> +is not disabled in xl.conf (see xl.conf(5) man page for more
information
>> +on this option). Specifying another domain requires setting up a
driver
>> +domain which is outside the scope of this document.
>>  
>>  ### rate
>>  
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 8ea3478..6b85cdc 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -2474,6 +2474,8 @@ out:
>>  int libxl__device_nic_setdefault(libxl__gc *gc, libxl_device_nic *nic,
>>                                   uint32_t domid)
>>  {
>> +    int run_hotplug_scripts;
>> +
>>      if (!nic->mtu)
>>          nic->mtu = 1492;
>>      if (!nic->model) {
>> @@ -2503,6 +2505,18 @@ int libxl__device_nic_setdefault(libxl__gc *gc,
libxl_device_nic *nic,
>>                                    libxl__xen_script_dir_path()) < 0
)
>>          return ERROR_FAIL;
>>  
>> +    run_hotplug_scripts = libxl__hotplug_settings(gc, XBT_NULL);
>> +    if (run_hotplug_scripts < 0) {
>> +        LOG(ERROR, "unable to get current hotplug scripts
execution setting");
> 
> Include the error value?
libxl__hotplug_settings already prints an error message, that contains
the errno value. I think that''s enough.
>> +        return run_hotplug_scripts;
>> +    }
>> +    if (nic->backend_domid != LIBXL_TOOLSTACK_DOMID &&
run_hotplug_scripts) {
>> +        LOG(ERROR, "the vif ''backend='' option
cannot be used in conjunction "
>> +                   "with run_hotplug_scripts, please set
run_hotplug_scripts "
>> +                   "to 0 in xl.conf");
> 
> This mention of xl.conf in libxl is a layering violation.
> 
> I think it would be fine for libxl to log something generic about
> hotplug scripts and return an error and to have a more specific check t
> parse time in xl which errors out with a reference to the config option.
Ok, I will probably split this in two patches, one for libxl and one for
the parser.

Thanks for the review.

On Thu, 2012-08-23 at 18:16 +0100, Roger Pau Monne
wrote:
> Ok, I will probably split this in two patches, one for libxl and one
> for the parser.
Are you still indenting to do this for 4.2.0? It''s fast approaching...

Ian.

Ian Campbell wrote:
> On Thu, 2012-08-23 at 18:16 +0100, Roger Pau Monne wrote:
>> Ok, I will probably split this in two patches, one for libxl and one
>> for the parser.
> 
> Are you still indenting to do this for 4.2.0? It''s fast
approaching...
Yes, sorry for the delay. I will try to submit a patch for it on Tuesday.