Re: [PATCH RESEND v3 1/2] libxl: postpone backend name resolution

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

On 04/12/2013 10:05 AM, Ian Campbell wrote:
> On Thu, 2013-03-14 at 14:23 +0000, Daniel De Graaf wrote:
>> This adds a backend_domname field in libxl devices that contain a
>> backend_domid field, allowing either a domid or a domain name to be
>> specified in the configuration structures.  The domain name is resolved
>> into a domain ID in the _setdefault function when adding the device.
>> This change allows the backend of the block devices to be specified
>> (which previously required passing the libxl_ctx down into the block
>> device parser), and will simplify specification of backend domains in
>> other users of libxl.
>
> please can we get a #define LIBXL_HAVE_DEVICE_BACKEND_NAME (or something
> similar) in libxl.h so applications can tell that it is safe to use
> this.

OK; I will use LIBXL_HAVE_DEVICE_BACKEND_DOMNAME to match the new field.

>
>> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Cc: Ian Campbell <ian.campbell@citrix.com>
>>
>> ---
>>
>> This patch does not include the changes to tools/libxl/libxlu_disk_l.c
>> and tools/libxl/libxlu_disk_l.h because the diffs contain unrelated
>> changes due to different generator versions.
>>
>>   docs/misc/xl-disk-configuration.txt | 12 +++++++++
>>   tools/libxl/libxl.c                 | 49 ++++++++++++++++++++++++++++++++++++-
>>   tools/libxl/libxl_types.idl         |  5 ++++
>>   tools/libxl/libxlu_disk_l.l         |  1 +
>>   tools/libxl/xl_cmdimpl.c            | 36 ++++++---------------------
>>   5 files changed, 73 insertions(+), 30 deletions(-)
>>
>> diff --git a/docs/misc/xl-disk-configuration.txt b/docs/misc/xl-disk-configuration.txt
>> index 86c16be..5bd456d 100644
>> --- a/docs/misc/xl-disk-configuration.txt
>> +++ b/docs/misc/xl-disk-configuration.txt
>> @@ -139,6 +139,18 @@ cdrom
>>   Convenience alias for "devtype=cdrom".
>>
>>
>> +backend=<domain-name>
>> +---------------------
>> +
>> +Description:           Designates a backend domain for the device
>> +Supported values:      Valid domain names
>> +Mandatory:             No
>> +
>> +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.
>> +
>> +
>>   backendtype=<backend-type>
>>   --------------------------
>>
>> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
>> index 73e0dc3..e442afc 100644
>> --- a/tools/libxl/libxl.c
>> +++ b/tools/libxl/libxl.c
>> @@ -1730,12 +1730,35 @@ static int libxl__device_nextid(libxl__gc *gc, uint32_t domid, char *device)
>>       return nextid;
>>   }
>>
>> +/* backend domain name-to-domid conversion utility */
>> +static int libxl__resolve_domain(libxl__gc *gc, const char *name)
>> +{
>> +    int i, rv;
>> +    uint32_t domid;
>> +    for (i=0; name[i]; i++) {
>> +        if (!isdigit(name[i])) {
>> +            rv = libxl_name_to_domid(libxl__gc_owner(gc), name, &domid);
>> +            if (rv)
>> +                return rv;
>> +            return domid;
>> +        }
>> +    }
>
> What are the constraints on the names of domains? I suppose it is
> toolstack specific but the xl docs jsut say it has to be unique.
>
> I'd suggest a simple rule like they cannot start with a digit, that
> would make the loop in the above unnecessary. In any case the loop is a
> bit odd since AFAICT it will treat "123boo" as "boo", I think it should
> either treat the entire string as a name or a number.

Actually, this will treat "123boo" properly, as name (not name+i) is passed
to libxl_name_to_domid.

> I think you should just move domain_qualifier_to_domid  from xl to libxl
> and use that...

That would also work, although it would need a better name:
libxl_qualifier_to_domid since libxl_name_to_domid is already used?

>> +    return atoi(name);
>> +}
>> +
>>   /******************************************************************************/
>>   int libxl__device_vtpm_setdefault(libxl__gc *gc, libxl_device_vtpm *vtpm)
>>   {
>> +   int rc;
>>      if(libxl_uuid_is_nil(&vtpm->uuid)) {
>>         libxl_uuid_generate(&vtpm->uuid);
>>      }
>> +   if (vtpm->backend_domname) {
>> +       rc = libxl__resolve_domain(gc, vtpm->backend_domname);
>> +       if (rc < 0)
>> +           return rc;
>> +       vtpm->backend_domid = rc;
>> +   }
>
> How about
> 	rc = libxl__resole_domain(gc, vtpm->backend_domname, &vtpm->backend_domid);
> 	if (rc < 0)
> 		return rc;
> ?
>
> Then the if (...) which is repeated could be in the function and it'd be
> easier to keep the logic the same for all of them.
>
> Ian.
>

This would conflict a bit with making libxl__resolve_domain public, although I
guess the function could note that it does nothing if passed a NULL string
which is all that is needed here.

-- 
Daniel De Graaf
National Security Agency