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

[Zhongze Liu <blackskygg@xxxxxxxxx>]

2017-08-08 18:49 GMT+08:00 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.

Yes, that's right. I'll turn the erros into warnings and proceed anyway.

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