Hi Wei,
Thank you for reviewing my patch.
2017-08-04 23:20 GMT+08:00 Wei Liu <wei.l...@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?
>
>> +
>> + if (NULL == libxl__xs_read(gc, xt, sshm_path)) {
>
> !libxl__xs_read
>
> We don't use "Yoda conditions".
I'll turn all the Yoda conditions into "natural" ones.
>
>> + 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.
OK. I'll move the entries out of the iteration. And yes, using an
infinite loop and
break on appropriate conditions will make it more concise.
>
>> + /* 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?
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.
>
>> + * 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?
>
>> + }
>> + 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?
Yes, the "sharing" here simply means adding one physical page to the slave
domains stage-2 mapping. The only thing that's changed is the refcount,
and I rely one the teardown code to decrease the refcount, which is implemented
on ARM but unimplemented on x86.
>
>> + }
>> +
>> + 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.
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.
>
>> + return s;
>> +}
>> +
>> int libxl__xs_read_mandatory(libxl__gc *gc, xs_transaction_t t,
>> const char *path, const char **result_out)
>> {
>> --
>> 2.13.3
>>
Cheers,
Zhongze Liu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
