Re: [PATCH] libxl: fix usage of backend parameter and run_hotplug_scripts

[Roger Pau Monne <roger.pau@citrix.com>]

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.