[PATCH] pid files cleanup

[PATCH] pid files cleanup

[Fabio M. Di Nitto]

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] pid files cleanup

[Alasdair G Kergon]

On Fri, Jul 09, 2010 at 11:04:04AM +0200, Fabio M. Di Nitto wrote:
> +	/* Create pidfile */
> +	if (create_lockfile(CLVMD_PIDFILE) < 0) {

For functions in lvm, our convention is to return 1 on success or 0 on failure.

> +++ 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?

> +++ b/lib/misc/lvm-file.c
> +	return 0;

return 1;

> +	return -errno;

return 0;

(This function already handled/reported errno appropriately.)

Looks good - ack.

Alasdair

[PATCH] pid files cleanup

[Fabio M. Di Nitto]

On 7/9/2010 3:09 PM, Alasdair G Kergon wrote:
> On Fri, Jul 09, 2010 at 11:04:04AM +0200, Fabio M. Di Nitto wrote:
>> +	/* 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.

> 
>> +++ 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.

> 
>> +++ b/lib/misc/lvm-file.c
>> +	return 0;
> 
> return 1;
> 
>> +	return -errno;
> 
> return 0;
> 
> (This function already handled/reported errno appropriately.)

I?ll check again.

> 
> Looks good - ack.

Ok thanks, I?ll repost the new patch with the correct return codes later
today or early monday morning.

Fabio

[PATCH] pid files cleanup

[Alasdair G Kergon]

On Fri, Jul 09, 2010 at 11:04:04AM +0200, Fabio M. Di Nitto wrote:
> - 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] pid files cleanup

[Fabio M. Di Nitto]

On 7/9/2010 3:11 PM, Alasdair G Kergon wrote:
> On Fri, Jul 09, 2010 at 11:04:04AM +0200, Fabio M. Di Nitto wrote:
>> - 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

Ok. Unless there is more than one dm-daemon, there is no benefit to move
it there.

Fabio

[PATCH] pid files cleanup

[Alasdair G Kergon]

On Fri, Jul 09, 2010 at 03:20:07PM +0200, Fabio M. Di Nitto wrote:
> 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] pid files cleanup

[Alasdair G Kergon]

On Fri, Jul 09, 2010 at 11:04:04AM +0200, Fabio M. Di Nitto wrote:
> - 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.
 
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