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
