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

[Wei Liu <wei.liu2@xxxxxxxxxx>]

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.

> >> +char *libxl__xs_get_sshmpath(libxl__gc *gc, const char *id)
> >> +{
> >> +    char *s = GCSPRINTF("/local/static_shm/%s", id);
> >> +    if (!s)
> >> +        LOGE(ERROR, "cannot allocate static shm path");
> >
> > GCSPRINTF can't fail. You can eliminate the check.
> 
> I was also confused about the other similar checks around the file
> since GCSPRINTF will die on failure. Em...It seems that I've copied
> the previous errors.
> 
> Then I'll remove this function and replace it with a macro in
> libxl_sshm.c instead.
> 

Using a static inline function is better.

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