Re: [libvirt] [PATCH 1/3] Add a plugin dlm for lock manager

On 02/05/2018 04:07 AM, river wrote: This is lacking in any description of the design or what this particular patch is accomplishing - although there is some of that in the cover. I imagine it would take too long to write up everything this single patch does though... According to how we prefer individuals to identify themselves for libvirt patches to be pushed, 'river' is not preferred. A legal name is preferred - there's plenty of examples in libvirt.git... There are a number of other guidelines at: https://libvirt.org/hacking.html One of the guidelines is to "Split large changes into a series of smaller patches, self-contained if possible, with an explanation of each patch and an explanation of how the sequence of patches fits together." - seeing a 1056 line C file and multiple m4 files makes me wonder if things could be split up a bit more... Furthermore it states to ensure to run certain tests - based on my git am of your patches and subsequent attempt to build, I can say for certainty you did not do. I don't even have to have the dlm and cpg installed to figure that out. Finally - fair warning - I used to work in OpenVMS development. It's been a while since I've thought about the OpenVMS Lock Manager; however, I was quite familiar with the concepts when I worked there as it was a large part of the project I worked on (one task of the tool was to help track/manage/fix lock issues). I then was in Tru64 Unix which essentially "rewrote" the OpenVMS Lock Manager into what it called DLM and I assume ends up being the basis for the open source version. In any case, whether you can "shoehorn" the dlm world/model into the virtlockd world/model for the usage of "cluster wide" resource management - I dunno. Lots of nitpick details follow - perhaps hard to follow. I tried not to repeat too much, but there are certainly some things that were so pervasive that I had to leave it up to you to go back through and deal with. I tried to reread after writing, but it's been a long day, I'm tired, and I have some other things to get to. Still I figured at least a looksee would be beneficial. There's no update to docs/locking.html.in nor creation of a docs/locking-dlm.html.in... That's where I'd expect to find the details of why someone would want to choose dlm over lockd or sanlock. What goodies are provided? What advantage is there? FWIW: configure, m4, and make files are not an area I know well enough to comment... Still seems like there's a dlm kernel model required, a libdlm required, and some sort of cluster wide id thing (cpg). UNKNOWN doesn't make it very useful does it? Is somewhere in /tmp the best/only place to put this? What not similar to lockd/sanlock using /var/lib/libvirt/dlm... (which would seemingly need to be created on installation). So the two modes will be "Protected Read" (shared) and "Exclusive" (unshared). My recollection is a resource took a NL (e.g., I'm interested) lock, then would promote/demote from there as it used/needed a resource. Having the NL is perhaps especially useful to know when the resource in question has had some other adjustment to it. In the OpenVMS world, you could define a function to be called when some other process/node in the environment became interested or requested a higher mode (those details are way too foggy to remember). Not really quite sure what using PR and EX will really buy you in the way of "features". Something w/ PR will block EX until PR is dropped (and vice versa). Depending on "how" you envision things to work, I would think you could use NL, CR, PR, and PW rather usefully. Yuck - for a stack variable... This should be closer to the VIR_FROM_THIS - it's lost in the middle here. Instead of "Dlm" why not use "DLM" in names? This is quite pervasive. Could just have gone with _virLockInfo to cut down on chars to type. I would think rather than (potentially long) linked lists, using hash tables would be a bit more efficient. As would using the libvirt object model. I would think uint32_t would be limiting... Why not just unsigned int? Alignment problems - looks like usage of a <tab> instead of spaces. Multiple globals? Why aren't the last 2 part of the @driver? Not a single comment for any of this functions - they're not all self documenting. Each should provide some sort of hint about arguments, purpose, and returns. Not just this function either. None of your code uses our more standard declaration syntax: static int virLockManagerDlmLoadConfig(const char *configFile) Also, I think the config "feature" should be introduced in a separate patch that would include the config stuff from patch 3... > +{ > + virConfPtr conf = NULL; > + int ret = -1; > + if (access(configFile, R_OK) == -1) { > + if (errno != ENOENT) { > + virReportSystemError(errno, > + _("Unable to access config file %s"), > + configFile); > + return -1; > + } > + return 0; > + } > + if (!(conf = virConfReadFile(configFile, 0))) ^ The indention is wrong here. and really wrong here. > + if (virConfGetValueBool(conf, "auto_disk_leases", &driver->autoDiskLease) < 0) These are some really long lines - typically like to keep "around" 80 characters with perhaps 1-3 chars of "))) {" type overflow. Not everyone keeps 132 char wide screens > + goto cleanup; > + driver->requireLeaseForDisks = !driver->autoDiskLease; > + if (virConfGetValueBool(conf, "require_lease_for_disks", &driver->requireLeaseForDisks) < 0) > + goto cleanup; > + if (virConfGetValueBool(conf, "purge_lockspace", &driver->purgeLockspace) < 0) > + goto cleanup; > + if (virConfGetValueString(conf, "lockspace_name", &driver->lockspaceName) < 0) This leaks driver->lockspaceName from virLockManagerDlmInit > + goto cleanup; > + if (virConfGetValueString(conf, "lock_record_file_path", &driver->lockRecordFilePath) < 0)' The leaks driver->lockRecordFilePath from virLockManagerDlmInit > + goto cleanup; > + ret = 0; > + cleanup: > + virConfFree(conf); > + return ret; > +} Two blank lines between functions for new code is the new normal. > +static int virLockManagerDlmToModeUint(const char *token) > +{ > + if (STREQ(token, PRMODE)) > + return LKM_PRMODE; > + if (STREQ(token, EXMODE)) > + return LKM_EXMODE; > +} > +static const char *virLockManagerDlmToModeText(const uint32_t mode) > +{ > + switch (mode) { > + case LKM_PRMODE: > + return PRMODE; > + case LKM_EXMODE: > + return EXMODE; > + default: > + return NULL; > + } > +} I actually think these two should be implemented via the VIR_ENUM_{DECL|IMPL} macros. That would result in API's that would allow conversion between type and string (e.g. vir*ToString and vir*FromString) > +static virLockInformationPtr virLockManagerDlmRecordLock(const char *name, > + const uint32_t mode, > + const uint32_t lkid, > + const pid_t vm_pid) This feels "separable" as in something done after you've created the base code and then want to add in whatever "feature" or "functionality" this provides. > +{ > + virLockInformationPtr lock = NULL; > + if (VIR_ALLOC(lock) < 0) > + goto error;> + if (VIR_STRDUP(lock->name, name) < 0) > + goto error; > + lock->mode = mode; > + lock->lkid = lkid; > + lock->vm_pid = vm_pid; > + virMutexLock(&(lockListWait.listMutex)); > + virListAddTail(&lock->entry, &(lockListWait.list)); > + virMutexUnlock(&(lockListWait.listMutex)); What is this list for? I think this also is completely separable. I wouldn't use a list either... 10s of domains probably works fine. 100s of domains perhaps not as well 1000s of domains starts being a bottleneck for sure. > + VIR_DEBUG("record lock sucessfully, lockName=%s lockMode=%s lockId=%d", successfully > + NULLSTR(name), NULLSTR(virLockManagerDlmToModeText(mode)), lkid); > + return lock; > + error: > + if (lock) > + VIR_FREE(lock->name); > + VIR_FREE(lock); Since you'll be having @lock contain something that needs VIR_FREE, implement a virLockDlmLockInfoFree type function that would free the lock, e.g.: if (!lock) return; VIR_FREE(lock->name); VIR_FREE(lock); > + return NULL; > +} > +static void virLockManagerDlmWriteLock(virLockInformationPtr lock, int fd, bool status) When you had multiple arguments in a previous decl you split them across multiple lines - should do the same here. Rather than a stack based length... Allocate on the fly. > + off_t offset = 0, rv = 0; > + if (!lock) { So is this because you don't want to support calling (NULL, fd, status) or that you don't want to support a @lock parameter that happens to be NULL "for some reason". If the former, then use ATTRIBUTE_NONNULL If the latter, then why is this a void function how do we know if it succeed or failed? > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("lock is NULL")); > + return; > + } > + /* > + * STATUS RESOURCE_NAME LOCK_MODE VM_PID\n > + * 6 64 9 10 > + * 93 = 6 + 1 + 64 + 1 + 9 + 1 + 10 + 1 Each of the +1 is a "separator" char ' '? And it's guaranteed somewhere that someone hasn't created/used a resource name of "happy gilmore". '*'?? Wait what? alignment issues (e.g. <tab>) Even more and different alignment issues. > + return; > + } > + snprintf(buffer, sizeof(buffer), "%6d %64s %9s %10jd\n", \ > + status, lock->name, > + NULLSTR(virLockManagerDlmToModeText(lock->mode)),> + (intmax_t)lock->vm_pid); The virAsprintf(buffer, would I think be a better mechanism... Having a NULL returned from *ToModeText would be killer wouldn't it? Why does the mode matter anyway in the buffer you're creating? An "intmax_t" does that work consistently and as expected everywhere? and 10 places only - would seem intmax could be larger than 10 numbers so that would seem to indicate some truncation could eventually happen thus causing algorithm failures. > + if (safewrite(fd, buffer, strlen(buffer)) != strlen(buffer)) { > + virReportSystemError(errno, > + _("unable to write lock information '%s' to file '%s'"), > + buffer, NULLSTR(driver->lockRecordFilePath)); > + return; > + } Not sure I understand the reason of the file > + VIR_DEBUG("write '%s' to fd=%d", buffer, fd); > + fdatasync(fd); > + return; How do we know if it failed? > +} > +static void virLockManagerDlmAdoptLock(char *raw) { > + char *str = NULL, *subtoken = NULL, *saveptr = NULL, *endptr = NULL; > + int i = 0, status = 0; > + char *name = NULL; > + uint32_t mode = 0; > + pid_t vm_pid = 0; > + struct dlm_lksb lksb = {0}; > + /* every line is the following format: > + * STATUS RESOURCE_NAME LOCK_MODE VM_PID > + */ See/use virStringSplit* I think it would be easier that whatever it is you're doing below. > + for (i = 0, str = raw, status = 0; ; str = NULL, i++) { > + subtoken = strtok_r(str, " \n", &saveptr); > + if (subtoken == NULL) > + break; > + switch(i) { > + case 0: These are magic numbers? > + if (virStrToLong_i(subtoken, &endptr, 10, &status) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot extract lock status '%s'"), subtoken); > + goto cleanup; > + } > + break; > + case 1: > + if (VIR_STRDUP(name, subtoken) != 1) > + goto cleanup; > + break; > + case 2: > + mode = virLockManagerDlmToModeUint(subtoken); > + if (!mode) > + goto cleanup; > + break; > + case 3: > + if ((virStrToLong_i(subtoken, &endptr, 10, &vm_pid) < 0) || !vm_pid) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot extract lock vm_pid '%s'"), subtoken); > + goto cleanup; > + } > + break; > + default: > + goto cleanup; > + break; > + } > + if (status != 1) > + goto cleanup; > + } > + if (i != 4) > + goto cleanup; > + /* copy from `lm_adopt_dlm` in daemons/lvmlockd/lvmlockd-dlm.c of lvm2: > + * dlm returns 0 for success, -EAGAIN if an orphan is bad indent > + * found with another mode, and -ENOENT if no orphan. > + * > + * cast/bast/param are (void *)1 because the kernel > + * returns errors if some are null. > + */ > + status = dlm_ls_lockx(lockspace, mode, &lksb, LKF_PERSISTENT|LKF_ORPHAN, > + name, strlen(name), 0, > + (void *)1, (void *)1, (void *)1, ^^ These would seem to relate to the same parametes used for dlm_lock[_wait]. They are actually quite useful when it comes to lock conversions. You may want to considering investigating that... Still seems like a "bug" to me to need to pass 1 as a callback function or a blocking callback function. Why not dlm_is_lock() instead since you're not using xid and timeout if (status < 0) or (status == -1) s/rv/status > + status, name, NULLSTR(virLockManagerDlmToModeText(mode))); > + goto cleanup; > + } > + if (!virLockManagerDlmRecordLock(name, mode, lksb.sb_lkid, vm_pid)) { Hmm.. I really don't understand the Recording yet. How can name be possible NULL here but non NULL above? (intmax_t) limited... > + } > + cleanup: > + if (name) Unnecessary to have if (name), VIR_FREE does that for you > + VIR_FREE(name);> + return; So how would any one know this is successful or not? > +} > +static int virLockManagerDlmPrepareLockList(const char *lockRecordFilePath) > +{ > + FILE *fp = NULL; > + int line = 0; > + size_t n = 0; > + ssize_t count = 0; > + char *buffer = NULL; > + fp = fopen(lockRecordFilePath, "r"); > + if (!fp) { > + virReportSystemError(errno, > + _("unable to open '%s'"), lockRecordFilePath); > + return -1; > + } > + /* lock information is from the second line */ The format of this file really needs to be documented somehow > + for (line = 0; !feof(fp); line++) { > + count = getline(&buffer, &n, fp); > + if (count <= 0) > + break; > + switch (line) { > + case 0: > + break; > + default: > + virLockManagerDlmAdoptLock(buffer); > + break; > + VIR_FORCE_FCLOSE(fp); > + VIR_FREE(buffer); > +} > +static int virLockManagerDlmGetLocalNodeId(uint32_t *nodeId) > +{ > + cpg_handle_t handle = 0; > + int rv = -1; > + if (cpg_model_initialize(&handle, CPG_MODEL_V1, NULL, NULL) != CS_OK) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unable to create a new connection to the CPG service")); alignment issue > + } > + if( cpg_local_get(handle, nodeId) != CS_OK) { if (cpg... Might be nice to know what error is returned xx = cpg* if (xx != CS_OK) { include xx in your output. > + goto cleanup; > + } > + VIR_DEBUG("the local nodeid=%u", *nodeId); > + rv = 0; > + cleanup: > + if (cpg_finalize(handle) != CS_OK) > + VIR_WARN("unable to finalize the CPG service"); but still OK to return @rv... > + return rv; > +} > +static int virLockManagerDlmDumpLockList(const char *lockRecordFilePath) > +{ > + virLockInformationPtr theLock = NULL; > + char buffer[BUFFERLEN] = {0}; Why a stack variable? > + int fd = -1, rv = -1; > + /* not need mutex because of only one instance would be initialized */ > + fd = open(lockRecordFilePath, O_WRONLY|O_CREAT|O_TRUNC, LOCK_RECORD_FILE_MODE); That's a very strange set of O_* values for a dump function. > + if (fd < 0) { > + virReportSystemError(errno, > + _("unable to open '%s'"), > + lockRecordFilePath); > + return -1; > + } > + snprintf(buffer, sizeof(buffer), "%6s %64s %9s %10s\n", \ Why the \ Again virAsprintf > + if (safewrite(fd, buffer, strlen(buffer)) != strlen(buffer)) { > + virReportSystemError(errno, > + _("unable to write '%s' to '%s'"), > + buffer, lockRecordFilePath); > + goto cleanup; > + } > + virListForEachEntry(theLock, &(lockListWait.list), entry) { > + virLockManagerDlmWriteLock(theLock, fd, 1); > + } No need for the { } because just one line... There are those that really don't like that type of obfuscation. > + if (VIR_CLOSE(fd) < 0) { > + virReportSystemError(errno, > + _("unable to close file '%s'"), > + lockRecordFilePath); > + goto cleanup; > + } > + rv = 0; > + cleanup: > + if (rv) > + VIR_FORCE_CLOSE(fd); > + return rv; > +} > +static int virLockManagerDlmSetupLockRecordFile(const char *lockRecordFilePath, > + const bool newLockspace, > + const bool purgeLockspace) > +{ > + uint32_t nodeId = 0; > + /* there maybe some orphan locks recorded in the lock record file which > + * should be adopted if lockspace is opened instead of created, we adopt > + * them then add them in the list. > + */ > + if (!newLockspace && > + virLockManagerDlmPrepareLockList(lockRecordFilePath)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unable to adopt locks from '%s'"), > + NULLSTR(lockRecordFilePath)); If @lockRecordFilePath cannot be passed as NULL, then just use ATTRIBUTE_NONNULL ... If though it's some variable that may be NULL, then <sigh>. > + return -1; > + } > + /* purgeLockspace flag means purging orphan locks belong to any process > + * in this lockspace. > + */ > + if (purgeLockspace && !virLockManagerDlmGetLocalNodeId(&nodeId)) { > + if (dlm_ls_purge(lockspace, nodeId, 0)) { > + VIR_WARN("node=%u purge DLM locks failed in lockspace=%s", > + nodeId, NULLSTR(driver->lockspaceName)); > + } > + else > + VIR_DEBUG("node=%u purge DLM locks success in lockspace=%s", > + nodeId, NULLSTR(driver->lockspaceName)); Both the if {} and the else need {} > + } > + /* initialize the lock record file */ > + if (virLockManagerDlmDumpLockList(lockRecordFilePath)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unable to initialize the lock record file '%s'"), > + lockRecordFilePath); > + return -1; > + } > +} > +static int virLockManagerDlmSetup(void) > +{ > + bool newLockspace = false; > + virListHeadInit(&(lockListWait.list)); > + if ((virMutexInit(&(lockListWait.listMutex)) < 0) || > + (virMutexInit(&(lockListWait.fileMutex)) < 0)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unable to initialize mutex")); > + return -1; > + } We have have an "internal" list of locks and use some sort of backing file for those, true? > + /* check whether dlm is running or not */ > + if (access(DLM_CLUSTER_NAME_PATH, F_OK)) { IOW: !virFileExists() > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("check dlm_controld, ensure it has setuped")); > + return -1; > + } > + /* open lockspace, create it if it doesn't exist */ > + lockspace = dlm_open_lockspace(driver->lockspaceName); > + if (!lockspace) { > + lockspace = dlm_create_lockspace(driver->lockspaceName, DLM_LOCKSPACE_MODE); > + if (!lockspace) { > + virReportSystemError(errno, "%s", > + _("unable to open and create DLM lockspace")); > + return -1; > + } > + newLockspace = true; > + } @lockspace should be stored in driver-> I would think. > + /* create thread to receive notification from kernel */ notification for what? what gets called? > + if (dlm_ls_pthread_init(lockspace)) { > + if (errno != EEXIST) { > + virReportSystemError(errno, "%s", > + _("unable to initialize lockspace")); > + return -1; > + /* we need file to record lock information used by rebooted libvirtd */ > + if (virLockManagerDlmSetupLockRecordFile(driver->lockRecordFilePath, > + newLockspace, > + driver->purgeLockspace)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unable to initialize DLM lock file")); Well not able to initialize the recording file technically. > + return -1; > + } > +} Above here seems to be able to be split from below here - probably even possible to create a something that doesn't do much in the first pass, then adds more functionality as we go along. More patches, but easier to review and read. IOW, multple patches please. Why a forward decl? Why not just define the Deinit first? > +static int virLockManagerDlmInit(unsigned int version, > + const char *configFile, > + unsigned int flags) > +{ > + VIR_DEBUG("version=%u configFile=%s flags=0x%x", version, NULLSTR(configFile), flags); > + virCheckFlags(0, -1); > + if (driver) > + return 0; > + if (geteuid() != 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("dlm lock requires root privileges")); > + return -1; > + } hmmm... system only and no support for session mode, something that'll need to be documented. > + if (VIR_ALLOC(driver) < 0) > + return -1; > + driver->autoDiskLease = true; > + driver->requireLeaseForDisks = !driver->autoDiskLease; > + driver->purgeLockspace = true; > + if (virAsprintf(&driver->lockspaceName, > + "%s", DLM_LOCKSPACE_NAME) < 0) > + goto error; This is nothing more than a VIR_STRDUP(driver->lockspaceName, DLM_LOCKSPACE_NAME)... ' > + if (virAsprintf(&driver->lockRecordFilePath, > + "%s", LOCK_RECORD_FILE_PATH) < 0) > + goto error; Likewise on building the default name. Eww... You're recording locks "forever"? How do we distinguish when locks are removed? > + if (virLockManagerDlmLoadConfig(configFile) < 0) > + goto error; > + if (virLockManagerDlmSetup() < 0) > + goto error; > + error: > + virLockManagerDlmDeinit(); > + return -1; > +} > +static int virLockManagerDlmDeinit(void) > +{ > + virLockInformationPtr theLock = NULL; > + if (!driver) > + return 0; > + if(lockspace) if (lockspace), although I would think @driver would be able to contain this rather than another global. > + dlm_close_lockspace(lockspace); > + /* not care about whether adopting lock or not, > + * just release those to prevent memory leak > + */ > + virListForEachEntry(theLock, &(lockListWait.list), entry) { > + virListDelete(&(theLock->entry)); > + VIR_FREE(theLock->name); > + VIR_FREE(theLock); This is of course where a virLockManagerDlmLockInfoFree(theLock) would come in handy. > + }' > + VIR_FREE(driver->lockspaceName); > + VIR_FREE(driver->lockRecordFilePath); Will a botched Record file cause an unending loop of Init/Deinit? > + VIR_FREE(driver); > +} > +static int virLockManagerDlmNew(virLockManagerPtr lock, > + unsigned int type, > + size_t nparams, > + virLockManagerParamPtr params, > + unsigned int flags) > +{ > + virLockManagerDlmPrivatePtr priv = NULL; > + size_t i; > + virCheckFlags(VIR_LOCK_MANAGER_NEW_STARTED, -1); > + if (!driver) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("dlm plugin is not initialized")); > + return -1; > + } > + if (type != VIR_LOCK_MANAGER_OBJECT_TYPE_DOMAIN) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unsupported object type %d"), type); > + return -1; > + } See I prefer the _lockd implementation which uses a case over the _sanlock implementation which does this same > + if (VIR_ALLOC(priv) < 0) > + return -1; @priv is leaked if return -1 in subsequent code. > + for (i = 0; i< nparams; i++) { > + if (STREQ(params[i].key, "uuid")) { > + memcpy(priv->vm_uuid, params[i].value.uuid, VIR_UUID_BUFLEN); > + } else if (STREQ(params[i].key, "name")) { > + if (VIR_STRDUP(priv->vm_name, params[i].value.str) < 0) > + return -1; > + } else if (STREQ(params[i].key, "id")) { > + priv->vm_id = params[i].value.ui; > + } else if (STREQ(params[i].key, "pid")) { > + priv->vm_pid = params[i].value.iv; > + } else if (STREQ(params[i].key, "uri")) { > + /* there would be a warning in some case according to the history patch, > + * so ignored > + */ > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unexpected parameter %s for object"), > + params[i].key); but let's continue anyway <sigh> - similar to _lockd code. It's too bad the _sanlock code didn't make the decision to somehow unify this code. Nonetheless, perhaps that's a good single "pre" patch for this series in order to reduce the cut-copy-paste. Yes, I know "uri" causes issues. It's not that difficult to figure out. > + /* check the following to prevent some unexpexted state in some case */ > + if (priv->vm_pid == 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing PID parameter for domain object")); > + return -1; > + } > + if (!priv->vm_name) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing name parameter for domain object")); > + return -1; > + } > + if (priv->vm_id == 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing ID parameter for domain object")); > + return -1; > + } > + if (!virUUIDIsValid(priv->vm_uuid)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("missing UUID parameter for domain object")); > + return -1; > + } Likewise, the error checking would seem to be able to be combined. Scary that the _sanlock code doesn't do much.. err.. any error checking on whether these parameters we found. > + lock->privateData = priv; > +} > +static void virLockManagerDlmFree(virLockManagerPtr lock) This would seem to be more of a PrivateFree type call... > +{ > + virLockManagerDlmPrivatePtr priv = lock->privateData; > + size_t i; > + if (!priv) > + return; > + for (i = 0; i < priv->nresources; i++) > + VIR_FREE(priv->resources[i].name); > + VIR_FREE(priv->resources); > + VIR_FREE(priv->vm_name); > + VIR_FREE(priv); > + lock->privateData = NULL; > + return; > +} > +static int virLockManagerDlmAddResource(virLockManagerPtr lock, > + unsigned int type, const char *name, Each param on it's own line > + size_t nparams, > + virLockManagerParamPtr params, > + unsigned int flags) > +{ > + virLockManagerDlmPrivatePtr priv = lock->privateData; > + char *newName = NULL; > + virCheckFlags(VIR_LOCK_MANAGER_RESOURCE_READONLY | > + VIR_LOCK_MANAGER_RESOURCE_SHARED, -1); > + /* Treat read only resources as a no-op lock request */ Why couldn't these be CR or NL locks? That way you can track all locks for everything... I assume no chance they convert to write at some time. > + if (flags & VIR_LOCK_MANAGER_RESOURCE_READONLY) > + return 0; BTW: I think the _sanlock implementation is a bit nicer with the helper functions to add the disk or lease resource. > + switch (type) { > + case VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK: > + if (params || nparams) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("unexpected parameters for disk resource")); > + return -1; > + } > + if (!driver->autoDiskLease) { > + if (!(flags & (VIR_LOCK_MANAGER_RESOURCE_SHARED | > + VIR_LOCK_MANAGER_RESOURCE_READONLY))) { So we earlier bailed because READONLY was set, so why are we checking this again? > + priv->hasRWDisks = true; > + /* ignore disk resource without error */ > + return 0; > + } > + } > + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) Interesting _lockd uses SHA256 and _sanlock uses MD5... > + goto cleanup; > + break; > + case VIR_LOCK_MANAGER_RESOURCE_TYPE_LEASE: hmm... considering the @params and @nparams check in the other case, would this case need to check if they exist or even use them? Seems the _lockd and _sanlock code does something with them. Although I like the conf variables earlier, I think some "common code" could be generated. Neither _lockd nor _sandisk do any sort of Hash on this name. > + break; > + default: > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("unknown lock manager object type %d"), > + type); Incorrect indents > + return -1; > + } > + if (VIR_EXPAND_N(priv->resources, priv->nresources, 1) < 0) > + goto cleanup; > + priv->resources[priv->nresources-1].name = newName; Personally not a fan of possibly long linked lists. I like the hash table model. Create a small initial hash table and then add/remove elements. So there's no check if there's the same name in the list - we just happily add one? And this would be a VIR_STEAL_PTR > + if (!!(flags & VIR_LOCK_MANAGER_RESOURCE_SHARED)) Ew !! > + priv->resources[priv->nresources-1].mode = LKM_PRMODE; > + else > + priv->resources[priv->nresources-1].mode = LKM_EXMODE; Understanding how dlm lock transitions work - I'm having a hard time with just having 2 modes here. Seems to me to remove the best parts of the dlm code. You have a resource... at times you care to EX lock it for write, while others it's OK to be shared for read. It's an ebb and flow. Of course whether the callers really ascribe to the same model is something I'm less aware of as I haven't gone down the path of where/how the virLockManager* API's are used in the code recently. If you initialized an 'int ret = -1;' at the top and only set it to 0 here then you could fall through to cleanup too as long as you've used the VIR_STEAL_PTR above on newName. > + cleanup: > + VIR_FREE(newName); > + return -1; > +} > +static int virLockManagerDlmAcquire(virLockManagerPtr lock, > + const char *state ATTRIBUTE_UNUSED, > + unsigned int flags, > + virDomainLockFailureAction action ATTRIBUTE_UNUSED, > + int *fd) > +{ > + virLockManagerDlmPrivatePtr priv = lock->privateData; > + virLockInformationPtr theLock = NULL; > + struct dlm_lksb lksb = {0}; > + int rv = -1, theFd = -1; > + size_t i; > + virCheckFlags(VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY | > + VIR_LOCK_MANAGER_ACQUIRE_RESTRICT, -1); > + /* allowed to start a guest which has read/write disks, but without any leases */ > + if (priv->nresources == 0 && > + priv->hasRWDisks && > + driver->requireLeaseForDisks) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("read/write, exclusive access, disks were present, but no leases specified")); > + return -1; > + } > + /* accorting to git patch history, add `fd` parameter in order to according But you just set it to -1??! > + if(!lockspace) { if (!lockspace) or driver->lockspace. > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("lockspace is not opened")); > + return -1; > + } > + if (!(flags & VIR_LOCK_MANAGER_ACQUIRE_REGISTER_ONLY)) { > + VIR_DEBUG("Acquiring object %zu", priv->nresources); > + theFd = open(driver->lockRecordFilePath, O_RDWR); > + if (theFd < 0) { > + virReportSystemError(errno, > + _("unable to open '%s'"), driver->lockRecordFilePath); > + return -1; > + } > + for (i = 0; i < priv->nresources; i++) { > + VIR_DEBUG("Acquiring object %zu", priv->nresources); > + memset(&lksb, 0, sizeof(lksb)); > + rv = dlm_ls_lock_wait(lockspace, priv->resources[i].mode, > + &lksb, LKF_NOQUEUE|LKF_PERSISTENT, > + priv->resources[i].name, strlen(priv->resources[i].name), This is where I wonder why we cannot pass the path to the disk to dlm instead of the hash So this function allows the NULL parameters but the other one required (void *)1 - something doesn't seem right. If one side of the if else uses {}, then both must > + /* rv would be 0 although acquiring lock failed */ > + rv = -1; > + goto cleanup; > + } > + theLock = virLockManagerDlmRecordLock(priv->resources[i].name, > + priv->resources[i].mode, > + lksb.sb_lkid, > + priv->vm_pid); > + if (!theLock) { > + virReportSystemError(errno, > + _("unable to record lock information, " > + "lockName=%s lockMode=%s lockId=%d vm_pid=%jd"), > + NULLSTR(priv->resources[i].name), Really it can be NULL? add a blank line to help readability Any dlm_ls* call should check "rv < 0" instead of "!rv" for failure as 0 is returned on success from how I'm reading the man pages... s/lockStatus/lksbStatus/ Well this certainly isn't good! > + rv = -1; > + goto cleanup; > + } > + virMutexLock(&(lockListWait.fileMutex)); > + virLockManagerDlmWriteLock(theLock, theFd, 1); > + virMutexUnlock(&(lockListWait.fileMutex)); > + } > + if(VIR_CLOSE(theFd) < 0) { if (VIR... > + virReportSystemError(errno, > + _("unable to save file '%s'"), > + driver->lockRecordFilePath); > + goto cleanup; > + if (flags & VIR_LOCK_MANAGER_ACQUIRE_RESTRICT) { > + /* no daemon watches this fd, do nothing here, just close lockspace before `execv` > + * `dlm_close_lockspace` always return 0, so ignore return value > + */ > + ignore_value(dlm_close_lockspace(lockspace)); > + lockspace = NULL; > + } > + rv = 0; > + cleanup: > + if (rv) > + VIR_FORCE_CLOSE(theFd); > + return rv; > +} > +static void virLockManagerDlmDeleteLock(const virLockInformationPtr lock, > + const char *lockRecordFilePath) > +{ > + int fd = -1; > + if (!lock) > + return; > + virMutexLock(&(lockListWait.listMutex)); > + virListDelete(&(lock->entry)); > + virMutexUnlock(&(lockListWait.listMutex)); > + fd = open(lockRecordFilePath, O_RDWR); > + if (fd < 0) { > + virReportSystemError(errno, > + _("unable to open '%s'"), lockRecordFilePath); > + goto cleanup; > + } > + virMutexLock(&(lockListWait.fileMutex)); > + virLockManagerDlmWriteLock(lock, fd, 0); Oh - the last parameter should be false here (and true in some other places)... Still not clear on the mgmt of that file. > + virMutexUnlock(&(lockListWait.fileMutex)); > + if (VIR_CLOSE(fd) < 0) { > + virReportSystemError(errno, > + _("unable to save file '%s'"), > + lockRecordFilePath); > + VIR_FORCE_CLOSE(fd); > + } > + cleanup: > + VIR_FREE(lock->name); > + VIR_FREE(lock); > +} > +static int virLockManagerDlmRelease(virLockManagerPtr lock, > + char **state, > + unsigned int flags) > +{ > + virLockManagerDlmPrivatePtr priv = lock->privateData; > + virLockManagerDlmResourcePtr resource = NULL; > + virLockInformationPtr theLock = NULL; > + struct dlm_lksb lksb = {0}; > + int rv = -1; > + size_t i; > + virCheckFlags(0, -1); > + if(state) if (state) > + *state = NULL; > + if(!lockspace) { if (!lockspace) > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("lockspace is not opened")); > + return -1; > + } > + for (i = 0; i < priv->nresources; i++) { > + resource = priv->resources + i; > + virListForEachEntry (theLock, &(lockListWait.list), entry) { > + if((theLock->vm_pid == priv->vm_pid) && > + STREQ(theLock->name, resource->name) && > + (theLock->mode == resource->mode)) { > + /* > + * there are some locks from adopting, the existence of `(void *)1` > + * when adopting makes 'terminated by signal SIGSEGV (Address > + * boundary error)' error appear. This is here for your purposes? So release is more like move to NL mode ... kind of what I was saying earlier I think for Acquire... You get a NL model lock and then you convert to PR/PW/EX whatever... > + &lksb, LKF_CONVERT, > + resource->name, > + strlen(resource->name), > + 0, NULL, NULL, NULL); > + if (rv < 0) { > + virReportSystemError(errno, > + _("failed to convert lock: rv=%d lockStatus=%d"), lksbStatus > + goto cleanup; > + } > + /* don't care whether the lock is released or not, > + * it will be automatically released after the libvirtd dead > + */ > + virLockManagerDlmDeleteLock(theLock, driver->lockRecordFilePath); I think we should care... Zero fault tolerance problably not good. > + rv = dlm_ls_unlock_wait(lockspace, lksb.sb_lkid, 0, &lksb); 0 for flags seems odd considering what you're trying to do (I think) lksbStatus > + goto cleanup; > + } > + break; > + } > + rv = 0; > + cleanup: > + return rv; > +} > +static int virLockManagerDlmInquire(virLockManagerPtr lock ATTRIBUTE_UNUSED, > + char **state, > + unsigned int flags) > +{ > + /* not support mannual lock, so this function almost does nothing */ 'manual' > + virCheckFlags(0, -1); > + if (state) > + *state = NULL; Why would this return NULL? My recollection is there is some sort of inquiry API for dlm. > +} > +virLockDriver virLockDriverImpl = > +{ > + .version = VIR_LOCK_MANAGER_VERSION, > + .flags = VIR_LOCK_MANAGER_USES_STATE, // currently not used Using // as a comment is something else that should be flagged by make syntax-check > + .drvInit = virLockManagerDlmInit, > + .drvDeinit = virLockManagerDlmDeinit, > + .drvNew = virLockManagerDlmNew, > + .drvFree = virLockManagerDlmFree, > + .drvAddResource = virLockManagerDlmAddResource, > + .drvAcquire = virLockManagerDlmAcquire, > + .drvRelease = virLockManagerDlmRelease, > + .drvInquire = virLockManagerDlmInquire, > +}; The following in particular is *easily* extractible into its own patch to be considered on its own for inclusion. If it were, there's lots of existing code that I would think could use something like this - although that's probably more than you want to be doing. None of the following are documented... > diff --git a/src/util/virlist.h b/src/util/virlist.h > new file mode 100644 > index 000000000..4ac3626c7 > --- /dev/null > +++ b/src/util/virlist.h > @@ -0,0 +1,110 @@ > +/* > + * virlist.h: methods for managing list, logic is copied from Linux Kernel > + * > + * Copyright (C) 2018 SUSE LINUX Products, Beijing, China. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2.1 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library. If not, see > + * <http://www.gnu.org/licenses/>;. > + * > + */ > +#ifndef __VIR_LIST_H > +#define __VIR_LIST_H > +#include <stdbool.h> > +typedef struct _virListHead virListHead; > +typedef virListHead *virListHeadPtr; > +struct _virListHead { > + struct _virListHead *next, *prev; > +}; > +static inline void > +virListHeadInit(virListHeadPtr name) > +{ > + name->next = name; > + name->prev = name; > +} > +static inline void > +__virListAdd(virListHeadPtr entry, > + virListHeadPtr prev, virListHeadPtr next) > +{ > + next->prev = entry; > + entry->next = next; > + entry->prev = prev; > + prev->next = entry; > +} > +static inline void > +virListAdd(virListHeadPtr entry, virListHeadPtr head) > +{ > + __virListAdd(entry, head, head->next); > +} > +static inline void > +virListAddTail(virListHeadPtr entry, virListHeadPtr head) > +{ > + __virListAdd(entry, head->prev, head); > +} > +static inline void > +__virListDelete(virListHeadPtr prev, virListHeadPtr next) > +{ > + next->prev = prev; > + prev->next = next; > +} > +static inline void > +virListDelete(virListHeadPtr entry) More like Remove than Delete... Delete to me implies @entry could be deleted too > +{ > + __virListDelete(entry->prev, entry->next); > +} > +static inline bool > +virListEmpty(virListHeadPtr head) > +{ > + return head->next == head; > +} > +#ifndef virContainerOf > +#define virContainerOf(ptr, type, member) \ > + (type *)((char *)(ptr) - (char *) &((type *)0)->member) > +#endif > +#define virListEntry(ptr, type, member) \ > + virContainerOf(ptr, type, member) > +#define virListFirstEntry(ptr, type, member) \ > + virListEntry((ptr)->next, type, member) > +#define virListLastEntry(ptr, type, member) \ > + virListEntry((ptr)->prev, type, member) > +#define __virContainerOf(ptr, sample, member) \ Without a doubt the extra spaces between ) and \ should raise the ire of the check-syntax test... > + (void *)virContainerOf((ptr), typeof(*(sample)), member) > +#define virListForEachEntry(pos, head, member) \ > + for (pos = __virContainerOf((head)->next, pos, member); \ > + &pos->member != (head); \ > + pos = __virContainerOf(pos->member.next, pos, member)) > +#define virListForEachEntrySafe(pos, tmp, head, member) \ Safe from what? John > + for (pos = __virContainerOf((head)->next, pos, member), \ > + tmp = __virContainerOf(pos->member.next, pos, member); \ > + &pos->member != (head); \ > + pos = tmp, tmp = __virContainerOf(pos->member.next, tmp, member)) > +#endif >