Re: [RFC PATCH 4/4] libxl: support creation and destruction of static shared memory areas

[Zhongze Liu <blackskygg@gmail.com>]

2017-08-08 18:56 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
> On Tue, Aug 08, 2017 at 11:49:35AM +0100, Wei Liu wrote:
>> On Sat, Aug 05, 2017 at 01:26:37AM +0800, Zhongze Liu wrote:
>> > Hi Wei,
>> >
>> > Thank you for reviewing my patch.
>> >
>> > 2017-08-04 23:20 GMT+08:00 Wei Liu <wei.liu2@citrix.com>:
>> > > I skim through this patch and have some questions.
>> > >
>> > > On Fri, Aug 04, 2017 at 10:20:25AM +0800, Zhongze Liu wrote:
>> > >> +
>> > >> +static int libxl__sshm_add_master(libxl__gc *gc, uint32_t domid,
>> > >> +                                  libxl_static_shm *sshm)
>> > >> +{
>> > >> +    int rc, aborting;
>> > >> +    char *sshm_path, *dom_path, *dom_role_path;
>> > >> +    char *ents[11];
>> > >> +    struct xs_permissions noperm;
>> > >> +    xs_transaction_t xt = XBT_NULL;
>> > >> +
>> > >> +    sshm_path = libxl__xs_get_sshmpath(gc, sshm->id);
>> > >> +    dom_path = libxl__xs_get_dompath(gc, domid);
>> > >> +    /* the domain should be in xenstore by now */
>> > >> +    assert(dom_path);
>> > >> +    dom_role_path = GCSPRINTF("%s/static_shm/%s/role", dom_path, sshm->id);
>> > >> +
>> > >> +
>> > >> + retry_transaction:
>> > >> +    /* Within the transaction, goto out by default means aborting */
>> > >> +    aborting = 1;
>> > >> +    rc = libxl__xs_transaction_start(gc, &xt);
>> > >> +    if (rc) { goto out; }
>> > >
>> > > if (rc) goto out;
>> >
>> > OK. Will remove all the {}. BTW, do I have to place "goto out;" in a newline?
>> >
>>
>> Youc can look for examples in existing code and follow those.
>>
>> [...]
>> > >> +static int libxl__sshm_del_single(libxl__gc *gc, xs_transaction_t xt,
>> > >> +                                  uint32_t domid, const char *id, bool master)
>> > >> +{
>> > >> +    char *sshm_path, *slaves_path;
>> > >> +
>> > >> +    sshm_path = libxl__xs_get_sshmpath(gc, id);
>> > >> +    slaves_path = GCSPRINTF("%s/slaves", sshm_path);
>> > >> +
>> > >> +    if (master) {
>> > >> +        /* we know that domid can't be both a master and a slave for one id,
>> > >
>> > > Is this enforced in code?
>> >
>> > Yes...and...no. I've done this in libxl__sshm_add_slave() by doing:
>> >
>> > +        if (NULL != libxl__xs_read(gc, xt, dom_sshm_path)) {
>> > +                    SSHM_ERROR(domid, sshm->id,
>> > +                               "domain tried to share the same region twice.");
>> > +                    rc = ERROR_FAIL;
>> > +                    goto out;
>> > +        }
>> >
>> > Maybe the comment is a little bit confusing. What I was planning to do is to
>> > place such a check in both *_add_slave() and *_add_master(), so that one
>> > ID can't appear twice within one domain, so that one domain will not be able
>> > to be both a master and a slave.
>> >
>>
>> OK this sounds plausible.
>>
>> > >
>> > >> +         * so the number of slaves won't change during iteration. Simply check
>> > >> +         * sshm_path/slavea to tell if there are still living slaves. */
>> > >> +        if (NULL != libxl__xs_read(gc, xt, slaves_path)) {
>> > >> +            SSHM_ERROR(domid, id,
>> > >> +                       "can't remove master when there are living slaves.");
>> > >> +            return ERROR_FAIL;
>> > >
>> > > Isn't this going to leave a half-destructed domain in userspace
>> > > components? Maybe we should proceed anyway?
>> >
>> > This is also among the points that I'm not very sure. What is the best way
>> > to handle this type of error during domain destruction?
>> >
>>
>> I think we should destroy everything in relation to the guest in Dom0
>> (or other service domains). Some pages for the master guests might be
>> referenced by slaves, but they will eventually be freed (hence the
>> domain struct will be freed) within Xen. Do experiment with this to see
>> if my prediction is right.
>>
>> It also occurs to me you need to guard against circular references. That
>> is, DomA and DomB have a mutual master-slave relationship.
>>
>
> This probably can't happen because you can't construct such pair of
> guests in the first place.

Yes, indeed. Because masters can only be created prior the slaves. So it must
be a forest-like structure.

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