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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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

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

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

> > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
> > index f3ae132..4aeb2bb 100644
> > --- a/tools/libxl/libxl_pci.c
> > +++ b/tools/libxl/libxl_pci.c
> > @@ -850,11 +850,14 @@ static int qemu_pci_add_xenstore(libxl__gc *gc, 
> > uint32_t domid,
> >      int rc = 0;
> >      char *path;
> >      char *state, *vdevfn;
> > +    uint32_t dm_domid;
> >  
> > -    path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", 
> > domid);
> > +    dm_domid = libxl_get_stubdom_id(CTX, domid);
> > +    path = libxl__sprintf(gc, "/local/domain/%d/device-model/%d/state",
> > +                          dm_domid, domid);
> 
> Maybe (up to you) switch thing to GCSPRINTF as you touch them?
> 

Sure.

Wei.

> Ian.

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