[dm-devel] [PATCH 03/21] libmultipath/checkers: replace message by msgid

[Martin Wilck]

On Thu, 2018-10-25 at 15:57 -0500,  Benjamin Marzinski  wrote:
> On Fri, Oct 12, 2018 at 12:26:49AM +0200, Martin Wilck wrote:
> > Replace the character array "message" in struct checker with
> > a "message ID" field.
> > 
> > The generic checker code defines a couple of standard message IDs
> > and corresponding messages. Checker-specific message IDs start
> > at CHECKER_FIRST_MSG. Checkers that implement specific message
> > IDs must provide a table for converting the IDs into actual log
> > messages.
> > 
> > This simplifies the checker data structure and the handling of
> > checker messages in the critical checker code path. It comes at
> > the cost that only constant message strings are supported. It
> > turns out that only a single checker log message (in the
> > emc_clariion
> > checker) was dynamically generated, and the missing information can
> > be provided with a standard condlog message.
> > 
> > Follow-up patches implement this for the existing checkers.
> 
> I have some cosmetic nits that I'll add inline, but none of them are
> real problems so, 
> 
> Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> > 
> [...]
> > +
> > +const char *checker_message(const struct checker *c)
> > +{
> > +	static char buf[CHECKER_MSG_LEN];
> > +	const char *msg = _checker_message(c);
> > +
> > +	if (msg == NULL || *msg == '\0')
> > +		*buf = '\0';
> > +	else
> > +		snprintf(buf, sizeof(buf), "%s checker%s",
> > +			 c->name, msg);
> > +	return buf;
> >  }
> 
> Since we are returning a pointer to a static buffer, it is
> theoretically
> possible for multiple threads to be updating the buffer at the same
> time. In reality, we are always calling checker_message with the vecs
> lock held, seeing as we need to guarantee that the path we are
> checking
> doesn't get deleted from pathvec while we are using it. However, it
> is
> easy to imagine future code calling checker_message for a path that
> hasn't been added to pathvec, and thus couldn't be deleted by another
> thread. In this case we wouldn't need the vecs lock, except to
> guarantee
> that we are the only thread using the static buffer. I don't really
> have
> a problem with keeping the static buffer, but we should add a comment
> that checker_message needs to be called with the vecs lock held.

Where to put that comment? Next to the function itself would be
pointless. Next to the definition of the vecs lock perhaps, where
people who work on the locking are likely to look?

>   
> >  void checker_clear_message (struct checker *c)
> >  {
> >  	if (!c)
> >  		return;
> > -	c->message[0] = '\0';
> > +	c->msgid = CHECKER_MSGID_NONE;
> >  }
> >  
> >  void checker_get (char *multipath_dir, struct checker * dst, char
> > * name)
> > @@ -288,10 +338,12 @@ void checker_get (char *multipath_dir, struct
> > checker * dst, char * name)
> >  	dst->fd = src->fd;
> >  	dst->sync = src->sync;
> >  	strncpy(dst->name, src->name, CHECKER_NAME_LEN);
> > -	strncpy(dst->message, src->message, CHECKER_MSG_LEN);
> > +	dst->msgid = src->msgid;
> 
> It looks odd setting dst->msgid to src->msgid, when we've never set
> src->msgid.  I realize that CHECKER_MSGID_NONE = 0, and we zero out
> the
> checker when we alloc is, so src->msgid is set to CHECKER_MSGID_NONE.
> But since we are always referring to msgid as a named constant, it
> would
> be clearer if we explicitly set it to CHECKER_MSGID_NONE in
> add_checker
> (even though it is unnecessary). This is purely cosmetic.

21/21 cleans this up.

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)