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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-04-12 at 15:41 +0100, Daniel De Graaf wrote:
> 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.

Sounds good.

> >> +    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.

So it is, cunning ;-)

> 
> > 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?

Maybe libxl_domain_qualifier_to... in case someone thinks of a qualifier
for something else?

> >> +    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.

I guess that's sensible behaviour in any case? I suppose the other
alternative would be to set domid => 0 which isn't want you want here
but could be what other potential callers want (e.g. the current xl
users of domain_qualifier_to_domid I guess).

I'm happy to leave this up to you depending on whether this suggested
change makes the xl callers worse or not and/or your preference.

Ian.


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