Re: [PATCH 3/5] libxl: use new QEMU xenstore protocol

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-03-19 at 10:36 +0000, Wei Liu wrote:
> On Wed, Mar 18, 2015 at 01:30:40PM +0000, Ian Campbell wrote:
> > On Fri, 2015-03-13 at 10:34 +0000, Wei Liu wrote:
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > index 3dedad4..4a38455 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -998,7 +998,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> > >      libxl_device_vfb *vfb;
> > >      libxl_device_vkb *vkb;
> > >      char **args;
> > > -    struct xs_permissions perm[2];
> > > +    struct xs_permissions perm[1];
> > >      xs_transaction_t t;
> > >  
> > >      /* convenience aliases */
> > > @@ -1106,16 +1106,16 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
> > >      }
> > >      xs_set_target(ctx->xsh, dm_domid, guest_domid);
> > >  
> > > -    perm[0].id = dm_domid;
> > > -    perm[0].perms = XS_PERM_NONE;
> > > -    perm[1].id = guest_domid;
> > > -    perm[1].perms = XS_PERM_READ;
> > > +    perm[0].id = guest_domid;
> > > +    perm[0].perms = XS_PERM_READ;
> > 
> > This will make the node owned by the guest (and hence r/w to the guest)
> > and set the perms for everyone else to r/o. I don't think this is what
> > you meant?
> > 
> > (The way perms are specified is a bit confusing, check out
> > http://wiki.xen.org/wiki/XenBus#Permissions )
> > 
> 
> Oh man! I completely misunderstood the semantics.

In fairness, they are insanely confusing, at least when encoded as an
array...

>  After reading that
> wiki page, I don't think I need to touch this permission bit because the
> original setting does what I need.
> 
> The content of that wiki page section should really be transferred to
> the comment before xs_set_permissions (which at the moment is extremely
> terse). I will send a patch to do that.

Awesome, thanks.

> > > -    return GCSPRINTF("/local/domain/0/device-model/%d/physmap/%s/%s",
> > > -            domid, phys_offset, node);
> > > +    return GCSPRINTF("/local/domain/%d/device-model/%d/physmap/%s/%s",
> > > +                     dm_domid, domid, phys_offset, node);
> > 
> > This sort of thing might imply that the helper takes the tail of the
> > path?
> 
> What do you mean? Sorry I don't follow.

I suggested before having a helper to return
"/local/domain/0/device-model/%d/", this hunk made me wonder if perhaps
that helper should take a "const char *fmt, ..." which it appends, so
you would call it as:

     foo(dm_domid, domid, "physmap/%s/%s", phys_offset, node)

or if you just want the base path for some reason
     foo(dm_domid, domid, "") (or NULL as the last parameter).

(I'm unclear if dm_domid and domid are both needed or if the funcution
can call get_stubdom_id(domid) internally, I'm sure you know...).

> > > SPRINTF(
> > > -                "/local/domain/0/device-model/%d/physmap", domid), &num);
> > > +                "/local/domain/%d/device-model/%d/physmap",
> > 
> > I only just noticed, but I think you want %u not %d (since dm_domid is
> > unsigned), although given the existing domid one is wrong too maybe you
> > don't want to bother...
> > 
> 
> Yes, I noticed but didn't bother to do that. "%d" is all over the place
> that it should be fixed all in one go...

Ack.