[Cluster-devel] [PATCH 4/5] dlm: add recovery callbacks

[David Teigland <teigland@redhat.com>]

On Mon, Dec 19, 2011 at 12:36:57PM +0000, Steven Whitehouse wrote:
> > +	struct dlm_lockspace_ops ls_ops;
>                     ^^^^^^^^^^ I'd suggest just keeping a pointer to
> this, see below.


> > +static int new_lockspace(const char *name, const char *cluster, uint32_t flags,
> > +			 int lvblen, struct dlm_lockspace_ops *ops,
>                                        ^^^^ this should be const


> > +	if (ops)
> > +		memcpy(&ls->ls_ops, ops, sizeof(struct dlm_lockspace_ops));
> > +
> Why not just keep a pointer to the ops? There is no need to copy them.
> 
> Also - since the ops are specific to a user of the lockspace, this
> implies that it is no longer possible to have multiple openers of the
> same lockspace on the same node. I think that needs documenting
> somewhere at least. The other possibility would be to introduce a
> structure to represent a "user of a lockspace" I suppose... 

> > +int dlm_new_lockspace(const char *name, const char *cluster, uint32_t flags,
> > +		      int lvblen, struct dlm_lockspace_ops *ops,
>                                     ^^^^ likewise this can also be const

> Please don't mix the callback arg and the functions in the same
> structure. The functions will be identical for all filesystems, where as
> the callback arg will be unique to each filesystem, so it would be
> better to just add an extra cb_arg to the new lockspace function.

I'll change all those, thanks