[Devel] Re: [PATCH 6/9][cr][v2]: Checkpoint file-locks

[Oren Laadan]

On Wed, 28 Jul 2010, Sukadev Bhattiprolu wrote:

> Oren Laadan [orenl at cs.columbia.edu] wrote:
> >
> >
> > Sukadev Bhattiprolu wrote:
> >> Oren Laadan [orenl at cs.columbia.edu] wrote:
> >> | | | > +	if (lock) {
> >> | > +		h->fl_start = lock->fl_start;
> >> | > +		h->fl_end = lock->fl_end;
> >> | > +		h->fl_type = lock->fl_type;
> >> | > +		h->fl_flags = lock->fl_flags;
> >> | > +	} else {
> >> | > +		/* Checkpoint a dummy lock as a marker */
> >> | > +		h->fl_start = -1;
> >> | | Maybe designate some constant for this ?  e.g. CKPT_FLOCK_NONE ?
> >> | In any case, you need a (loff_t) -1  (like in the restore code).
> >>
> >> Ok. Defining macros CKPT_HDR_SET_MARKER_LOCK() and
> >> CKPT_HDR_CHECK_MARKER_LOCK().  Also added the missing (loff_t).
> >
> > The nice thing about #define CKPT_FLOCK_NONE -1  - see also
> > the CKPT_PID_NULL, when defined in checkpoint_hdr.h, is that
> > they are intentionally visible to userspace too.
> 
> The problem is marker lock is currently identified by two fields:
> 
> 	.fl_start = -1;
> 	.fl_type = FL_POSIX.
> 
> so CKPT_FLOCK_NONE appears misleading if it only refers to one field.
> 
> Should I say CKPT_FLOCK_NONE_START and CKPT_FLOCK_NONE_TYPE ?
> 
> Or, can we make CKPT_CHECK_MARKER_LOCK() and CKPT_SET_MARKER_LOCK()
> available to user space ?

Having looked at the code again - how about the following:
get rid of this "last entry" altogether; instead, during
checkpoint, first count the locks, then write a header with
the number of locks, following by that many records of the
locks themselves.

This is what we do for other resource lists/chains as well.
Also makes it a bit easier to parse since you always know
what to expect once you see the header.

Oren.


_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers