[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 02/10] libxl_internal: functions to lock / unlock domain configuration
On Thu, Jul 24, 2014 at 07:24:10PM +0100, Ian Jackson wrote: > Wei Liu writes ("[PATCH v1 02/10] libxl_internal: functions to lock / unlock > domain configuration"): > > Simple file lock taken from xl to serialise access to "libxl-json" file. > > If a thread cannot get hold of the lock it waits due to F_SETLKW. > > Right. > > > In order to generate lock file name, rename userdata_path to > > libxl__userdata_path and declare it in libxl_internal.h > > I don't mind it in such a small patch but in general it is easier to > review things if non-functional changes like this are split out into a > separate patch. > Ack. > > +int libxl__lock_domain_configuration(libxl__gc *gc, uint32_t domid, > > + int *fd_lock) > > +{ > ... > > + int rc; > > + struct flock fl; > > + const char *lockfile; > > + > > + if (*fd_lock >= 0) > > + return ERROR_INVAL; > > Why not assert() ? > Ack. > > + lockfile = libxl__userdata_path(gc, domid, "libxl-json.lock", "d"); > > Perhaps lockfile = ...(, "libxl-json", "l") ? I think users of > libxl__userdata_path are entitled to invent their own `wh' values. > Good to know. > Otherwise you have to document "libxl-json.lock" as a reserved > userdata name (which is a bit daft because no-one would use it, but it > is, formally speaking, wrong to use it here). > I think I will go with the "l" approach. > > + *fd_lock = open(lockfile, O_WRONLY|O_CREAT, S_IWUSR); > > + if (*fd_lock < 0) { > > + LOGE(ERROR, "cannot open lockfile %s errno=%d\n", lockfile, errno); > > LOGE's message should not contain \n. > Fixed. > > + if (fcntl(*fd_lock, F_SETFD, FD_CLOEXEC) < 0) { > > What's wrong with libxl_fd_set_cloexec ? > Will use this one. This implementation was basically a copy of the one in xl, I didn't check if there's some helper function to do that. > > + close(*fd_lock); > > Please use the idempotent `goto out' error handling style to deal with > closing the fd on error. Your failure to do so has resulted in > error-case fd leak in this function. > Ack. > > +get_lock: > > + fl.l_type = F_WRLCK; > > + fl.l_whence = SEEK_SET; > > + fl.l_start = 0; > > + fl.l_len = 0; > > + rc = fcntl(*fd_lock, F_SETLKW, &fl); > > + if (rc < 0 && errno == EINTR) > > + goto get_lock; > > Please, no more of these `goto'-based loops! > Ack. > > + if (rc < 0) { > > + LOGE(ERROR, "cannot acquire lock %s errno=%d\n", lockfile, errno); > > + rc = ERROR_FAIL; > > goto out. > > > + } else > > + rc = 0; > > No, not like that. Like this: > > rc = 0; > return rc; > > out: > if (*fd_lock >= 0) { close(*fd_lock); *fd_lock = -1; } > return rc; > > Will fix this. > > +int libxl__unlock_domain_configuration(libxl__gc *gc, uint32_t domid, > > + int *fd_lock) > > +{ > > Closing the fd is sufficient. I'm not even sure why you need a whole > function for this; the caller could just call close(). The caller can > can ignore any errors (which I think are impossible anyway) since > after close the fd is gone anyway. > > > +/* > > + * Lock / unlock domain configuration in libxl private data store. > > + * fd_lock contains the file descriptor pointing to the lock file. > > + */ > > +int libxl__lock_domain_configuration(libxl__gc *gc, uint32_t domid, > > + int *fd_lock); > > You need to explain the lifetime semantics of *fd_lock. Your code > demands that the caller set it to -1 beforehand (which is fine). > Ack. > > +int libxl__unlock_domain_configuration(libxl__gc *gc, uint32_t domid, > > + int *fd_lock); > > If you do retain this as a separate function, it should return void. > I can think of nothing useful that the caller could do with an error > from it. > I will retain this as a separate function so that it looks symmetric to __lock. Wei. > Ian. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |