[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 3/5] libxl: use new QEMU xenstore protocol
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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |