From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Teigland Date: Mon, 19 Dec 2011 12:59:34 -0500 Subject: [Cluster-devel] [PATCH 4/5] dlm: add recovery callbacks In-Reply-To: <1324298217.2723.31.camel@menhir> References: <1324072991-30729-5-git-send-email-teigland@redhat.com> <1324298217.2723.31.camel@menhir> Message-ID: <20111219175934.GC24652@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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