[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC PATCH 4/4] libxl: support creation and destruction of static shared memory areas
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |