Re: [Xen-devel] [PATCH RFC 02/10] libxl: add new hotplug interface support to hotplug script callers

[Roger Pau Monnà <roger.pau@xxxxxxxxxx>]

On 18/01/13 14:29, Ian Campbell wrote:
> On Fri, 2012-12-21 at 17:00 +0000, Roger Pau Monne wrote:
>> Add support to call hotplug scripts that use the new hotplug interface
>> to the low-level functions in charge of calling hotplug scripts. This
>> new calling convention will be used when the hotplug_version aodev
>> field is 1.
> 
> Is there perhaps some way we can avoid users having to think about the
> hotplug_version?
> 
> For example if we export XENBUGS_PATH as a synonym for BACKEND_PATH and
> arrange that the new protocol involves calling action "add" and
> "remove" (as well as the other new ones) do old scripts mostly Just Work
> because they ignore the prepare/unprepare calls? (as a small sample
> vif-bridge and block-nbd seem to).

I'm afraid old scripts will return an error when called with commands
different than "add" or "remove", this is from block-common.sh:

if [ "$command" != "add" ] &&
   [ "$command" != "remove" ]
then
  log err "Invalid command: $command"
  exit 1
fi

> Alternatively by way of backwards compat perhaps we could provide a
> wrapper script? So if today you use script="/my-custom-script" you would
> switch to script="/etc/xen/hotplug-compat-wrapper /my-custom-script" and
> the wrapper would shim or ignore the new calls into the old?

Well, from a user point of view, this will still be the same, old
hotplug scripts will be called using the "script" config option, so
there will be no need to change configuration files. The only difference
is that when calling hotplug scripts using the new hotplug interface
users should use "method" instead of "script" in their config file, as
an example this would be the diskspec line to attach a disk using the
iSCSI block script:

'method=block-iscsi,vdev=xvda,target=iqn=iqn.1994-04.org.netbsd.iscsi-target:target0,portal=192.168.1.128'

Users don't have to deal directly with hotplug_version, I think forcing
them to user a wrapper is worse, because that will mean modifications to
config files when updating.

>>
>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> ---
>>  tools/libxl/libxl_device.c   |   33 +++++++++++++++++++-
>>  tools/libxl/libxl_internal.h |   33 ++++++++++++++++++--
>>  tools/libxl/libxl_linux.c    |   68 
>> ++++++++++++++++++++++++++++++++----------
>>  tools/libxl/libxl_netbsd.c   |    2 +-
>>  4 files changed, 115 insertions(+), 21 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
>> index 58d3f35..85c9953 100644
>> --- a/tools/libxl/libxl_device.c
>> +++ b/tools/libxl/libxl_device.c
>> @@ -83,6 +83,35 @@ out:
>>      return rc;
>>  }
>>
>> +char *libxl__device_hotplug_action(libxl__gc *gc,
>> +                                   libxl__device_action action)
>> +{
> 
> If you define libxl__device_action in
> tools/libxl/libxl_types_internal.idl you'll get this for free.

Thanks, will do that then.

> [...]
>> diff --git a/tools/libxl/libxl_netbsd.c b/tools/libxl/libxl_netbsd.c
>> index 9587833..8061e7a 100644
>> --- a/tools/libxl/libxl_netbsd.c
>> +++ b/tools/libxl/libxl_netbsd.c
>> @@ -62,7 +62,7 @@ out:
>>  int libxl__get_hotplug_script_info(libxl__gc *gc, libxl__device *dev,
>>                                     char ***args, char ***env,
>>                                     libxl__device_action action,
>> -                                   int num_exec)
>> +                                   int num_exec, int hotplug_version)
> 
> Is it worth wrapping verison and action into a little
> libxl__hotplug_action struct? 

It will slim down the list of parameters to pass to this function, which
is already too long for my taste.


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