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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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@xxxxxxxxxx>:
> > > 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.

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