pid files cleanup
PATCH 1/1 pid files cleanup
Hi all,
as previously discussed, the patch:
- adds a generic create_lockfile fn to lvm-file.{c,h}. Based on multipathd/pidfile.c
- adds --clvmd-pidfile configure option (autoreconf on F-12)
- make clvmd use the new create_lockfile
- make cmirrord use the new create_lockfile and drop duplicate code
- unlink pidfiles on exit (note that because of the atexit() behavior this has to be done locally in the daemon and cannot be generalized in the library).
NOTES:
- I had some issues to build dmeventd that I need to investigate locally. dmeventd also needs to be ported (if possible) to use the library implementation of create_lockfile().
- create_lockfile can probably use fcntl_lock_file, but I am not entirely sure about its behavior yet (specially the waiting note in the header), so I choose a safe path for now and use a known (to me) implementation.
- WHATS_NEW will be updated once patch is ACK.
Thanks
Fabio
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pid_files.diff
URL: <http://listman.redhat.com/archives/lvm-devel/attachments/20100709/b8252385/attachment.ksh>
PATCH 1/1 pid files cleanup
Review
Quoted 2 lines · Fabio M. Di Nitto
(quoted diff) +2
-0+ /* Create pidfile */ + if (create_lockfile(CLVMD_PIDFILE) < 0) {
For functions in lvm, our convention is to return 1 on success or 0 on failure.
Quoted 3 lines
+++ b/daemons/cmirrord/Makefile.in
+LVMLIBS = $(LVMINTERNAL_LIBS)
Hmmm.
We still have already:
cmirrord: $(OBJECTS) $(top_builddir)/lib/liblvm-internal.a
- Does the definition above mean this bit can come out?
Review
Quoted 2 lines
+++
b/lib/misc/lvm-file.c(quoted diff) +1
-0+ return 0;
return 1;
Review
Quoted 1 line
(quoted diff) +1
-0+ return -errno;
return 0;
(This function already handled/reported errno appropriately.)
Looks good - ack.
Alasdair
PATCH 1/1 pid files cleanup
Quoted 5 lines · Alasdair G Kergon
Review
Quoted 2 lines · Fabio M. Di Nitto
(quoted diff) +2
-0+ /* Create pidfile */ + if (create_lockfile(CLVMD_PIDFILE) < 0) {For functions in lvm, our convention is to return 1 on success or 0 on failure.
Ok, i?ll swap them around.
Quoted 11 lines
Quoted 4 lines
+++ b/daemons/cmirrord/Makefile.in
+LVMLIBS = $(LVMINTERNAL_LIBS)
Hmmm.
We still have already:
cmirrord: $(OBJECTS) $(top_builddir)/lib/liblvm-internal.a- Does the definition above mean this bit can come out?
It?s best to have them there both.
cmirrord: $(top_builddir)/lib/liblvm-internal.a
expresses a dependency that is required before calling the linker.
+LVMLIBS = $(LVMINTERNAL_LIBS)
this is necessary to link against liblvm-internal.
Quoted 11 lines
Review
Quoted 3 lines
+++
b/lib/misc/lvm-file.c(quoted diff) +1
-0+ return 0;return 1;
Review
Quoted 1 line
(quoted diff) +1
-0+ return -errno;return 0;
(This function already handled/reported errno appropriately.)
I?ll check again.
Quoted 2 lines
Looks good - ack.
Ok thanks, I?ll repost the new patch with the correct return codes later today or early monday morning.
Fabio
PATCH 1/1 pid files cleanup
In reply to Fabio M. Di NittoQuoted 3 lines · Fabio M. Di Nitto
- I had some issues to build dmeventd that I need to investigate locally. dmeventd also needs to be ported (if possible) to use the library implementation of
create_lockfile().
dmeventd is part of dm not lvm so has no access to lvm_internal!
For dmeventd to use the code, you need to move it into libdevmapper. The function then needs a dm_ prefix and goes into libdm-file.c
Alasdair
PATCH 1/1 pid files cleanup
Quoted 9 lines · Alasdair G Kergon
Quoted 3 lines · Fabio M. Di Nitto
- I had some issues to build dmeventd that I need to investigate locally. dmeventd also needs to be ported (if possible) to use the library implementation of
create_lockfile().dmeventd is part of dm not lvm so has no access to
lvm_internal!For dmeventd to use the code, you need to move it into libdevmapper. The function then needs a
dm_prefix and goes intolibdm-file.c
Ok. Unless there is more than one dm-daemon, there is no benefit to move it there.
Fabio
PATCH 1/1 pid files cleanup
Quoted 2 lines · Fabio M. Di Nitto
Ok. Unless there is more than one dm-daemon, there is no benefit to move it there.
Code like this that we want available to both dm components and LVM components we *move* to libdevmapper. So just take it out of lvm-file.c and put it into libdm-file.c with a dm_ prefix on the name and the fn prototype in libdevmapper.h in the "file/stream manipulation" section and a comment there that it's the caller's responsibility to unlink the file before exiting.
Alasdair
PATCH 1/1 pid files cleanup
In reply to Fabio M. Di NittoQuoted 4 lines · Fabio M. Di Nitto
-
create_lockfilecan probably usefcntl_lock_file, but I am not entirely sure about its behavior yet (specially the waiting note in the header), so I choose a safe path for now and use a known (to me) implementation.
Don't worry about that now if the code is moving to libdevmapper anyway. Sharing it just brings in the existing code that can create the directory path if it's not there, but it really always should be!
Alasdair