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

[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;

> +
> +    if (NULL == libxl__xs_read(gc, xt, sshm_path)) {

!libxl__xs_read

We don't use "Yoda conditions".

> +        rc = libxl__xs_write_checked(gc, xt, dom_role_path, "master");
> +        if (rc) { goto out; };
> +
> +        ents[0] = "master";
> +        ents[1] = GCSPRINTF("%"PRIu32, domid);
> +        ents[2] = "begin";
> +        ents[3] = GCSPRINTF("0x%"PRIx64, sshm->begin);
> +        ents[4] = "end";
> +        ents[5] = GCSPRINTF("0x%"PRIx64, sshm->end);
> +        ents[6] = "prot";
> +        ents[7] = libxl__strdup(gc, libxl_sshm_prot_to_string(sshm->prot));
> +        ents[8] = "cache_policy";
> +        ents[9] = libxl__strdup(gc,
> +                      libxl_sshm_cachepolicy_to_string(sshm->cache_policy));
> +        ents[10] = NULL;
> +

These aren't going to change from iteration to iteration, so you can
prepare them before entering the loop.

BTW it would be cleaner to use a for (;;) {} or while (1) {} loop to
implement this instead of using goto label. You can then eliminate the
TRY_TRANSACTION_OR_FAIL macro.

> +        /* could only be accessed by Dom0 */
> +        noperm.id = 0;
> +        noperm.perms = XS_PERM_NONE;
> +        libxl__xs_mknod(gc, xt, sshm_path, &noperm, 1);
> +        libxl__xs_writev(gc, xt, sshm_path, ents);
> +    } else {
> +        SSHM_ERROR(domid, sshm->id, "can only have one master.");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    aborting = rc = 0;
> +
> + out:
> +    TRY_TRANSACTION_OR_FAIL(aborting);
> +    return rc;
> +}
> +
[...]
> +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?

> +         * 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?

> +        }
> +        libxl__xs_path_cleanup(gc, xt, sshm_path);
> +    } else {
> +        libxl__xs_path_cleanup(gc, xt,
> +            GCSPRINTF("%s/%"PRIu32, slaves_path, domid));

Is this really enough? What will happen to the mapping? You merely
delete the xenstore node for it but the mapping is still there.

I suppose you're relying on the hypervisor to remove the mapping from
p2m?

> +    }
> +
> +    return 0;
> +}
> +
[...]
> diff --git a/tools/libxl/libxl_xshelp.c b/tools/libxl/libxl_xshelp.c
> index c4a18df353..d91fbf5fda 100644
> --- a/tools/libxl/libxl_xshelp.c
> +++ b/tools/libxl/libxl_xshelp.c
> @@ -193,6 +193,14 @@ char *libxl__xs_libxl_path(libxl__gc *gc, uint32_t domid)
>      return s;
>  }
>  
> +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.

> +    return s;
> +}
> +
>  int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
>                               const char *path, const char **result_out)
>  {
> -- 
> 2.13.3
> 

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