I could feel that you want to express "Oh, shit! How terrible the code it is." from the reply, although across the screen. Anyway, I'm really glad to thanks for your review and suggestions. :-)
On 2018年02月08日 07:55, John Ferlan wrote:

> 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? For those, assuming this scene: one customer has used cluster software with DLM in their environment, why not choose DLM plugin instead of sanlock/lockd ? Beside that, compared with sanlock/lockd, DLM don't need the share storage, and sanlock would bring a little heavyheavily on disk IO according to lockd RFC(2011-July/msg00337.html), but DLM only requires network. I have sent a RFC in 12/2017 to ack how the opinion is. Daniel said that "it can be considered in scope for a libvirt lock manager plugin if you want to try writing one"(2017-December/msg00706.html). So I draft this patch set which could only make DLM run in libvirt(with configure flag `--with-dlm` to compile 'dlm.so' module by force). Hope some suggestion bringed from libvirt community to help me improve, if accepted DLM lock manager plugin RFC, so I don't update 'docs/locking.html.in'. -------------

The following is the reply contents, aim to answer why I do that for most comments: 1) sorry for indent in my code. I coded them in two notebooks, both using vim. However one is newer, I have not configured '.vimrc' in that one. 2) >> > +AC_DEFUN([LIBVIRT_CHECK_CPG],[ >> > + dnl in some distribution, Version is `UNKNOW` in libcpg.pc > > UNKNOWN > doesn't make it very useful does it? In some distribution, for example, openSUSE Tumbleweed:    $ cat /usr/lib64/pkgconfig/libcpg.pc

    prefix=/usr
    exec_prefix=${prefix}
    libdir=/usr/lib64
    includedir=${prefix}/include

    Name: cpg
    Version: UNKNOWN
    Description: cpg
    Requires:
    Libs: -L${libdir} -lcpg
    Cflags: -I${includedir}
using `LIBVIRT_CHECK_PKG([CPG], [libcpg], [xxx])` directly would make build error. So I decide to ignore 'version'. I also don't know why that is. 3) >> > +#define LOCK_RECORD_FILE_PATH       "/tmp/libvirtd-dlm-file" > > 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). In my opinion, if one node reboots, DLM cluster would drop locks belong that node, most of Linux distribution would clean '/tmp' directory... 4) >> > + >> > +#define STATUS             "STATUS" >> > +#define RESOURCE_NAME      "RESOURCE_NAME" >> > +#define LOCK_ID            "LOCK_ID" >> > +#define LOCK_MODE          "LOCK_MODE" >> > +#define VM_PID             "VM_PID" >> > + >> > +#define BUFFERLEN          128 > > Yuck - for a stack variable... >> > +    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? Badly code wants to provided a readable file, like that:     STATUS                                                    RESOURCE_NAME LOCK_MODE     VM_PID        0 1501e418dedd122b050c91ec61650ae30d3c217806125d3c2f365989143aa28d    EXMODE      21857

The first line is the form header, the following is the lock information.

5) >> > + char *name; >> > + uint32_t mode; >> > + uint32_t lkid; > > I would think uint32_t would be limiting... Why not just unsigned int? view in '/usr/include/libdlm.h', it use `uint32_t` instead of `unsigned int`. 6) >> > + 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. Hashtable is a good idea, and I wanted to use it but I had never read it's code... Chosing list because that there won't be too much vms in one host, is it offten common that there is more than 50 vms in normal usage? 7) >> > + 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 Compared with sanlock and lockd, this implement of DLM has no daemon, some area is needed to record lock information in order to resume lock after libvirtd reboot. I chose file. 8) >> > + * 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. > >> > + NULL, NULL); > > Why not dlm_is_lock() instead since you're not using xid and timeout The code is from lvm2, I try to use `dlm_lock_wait`, however it failed, I don't know why. `(void *)1` is the same reason. Maybe a bug from libdlm or 'dlm.ko'. 9) >> > + >> > + /* create thread to receive notification from kernel */ > > notification for what? what gets called? This is implemented by libdlm, the result is writted in `lksb`. int dlm_lock_wait(uint32_t mode, struct dlm_lksb *lksb, uint32_t flags, const void *name, unsigned int namelen, uint32_t parent, /* unused */ void *bastarg, void (*bastaddr) (void *bastarg), void *range); /* unused */ 10) >> > + 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... > > >> > + /* we need format the lock information, so the lock name must be the constant length */ >> > + if (virCryptoHashString(VIR_CRYPTO_HASH_SHA256, name, &newName) < 0) >> > + goto cleanup; > > Neither _lockd nor _sandisk do any sort of Hash on this name. > > >> > + 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 It's interesting here. Originally I use MD5 instead of SHA256, but I found that: typedef enum { VIR_CRYPTO_HASH_MD5, /* Don't use this except for historic compat */ VIR_CRYPTO_HASH_SHA256, VIR_CRYPTO_HASH_LAST } virCryptoHash; Futhermore, I find the bug patch related sanlock name: sanlock's name length is limited 48, there is always someone use more than 48 characters (see https://bugzilla.redhat.com/show_bug.cgi?id=735443). The same situation, why not use the constant length? DLM limits name is 64 bytes to avoid some potential bugs. Besides, it's convenient to format write it to file. 11) >> > + * 'ensure sanlock socket is labelled with the VM process label', >> > + * however, fixing sanlock socket security labelling remove related >> > + * code. Now, `fd` parameter is useless. >> > + */ >> > + if (fd) >> > + *fd = -1; > > But you just set it to -1??! Maybe I can submit a patch to cut those code. Afterall it's useless now. 12) >> > + virCheckFlags(0, -1); >> > + >> > + if (state) >> > + *state = NULL; > > Why would this return NULL? My recollection is there is some sort of > inquiry API for dlm. Inquiry API is disappeared now... Only lock/unlock refered to libdlm source code and reference book. http://people.redhat.com/ccaulfie/docs/rhdlmbook.pdf Chapter 4.3. Lock Query Operations

-- 
Regards
Lin Fu(river)