[dm-devel] [PATCH 30/35] multipathd: uxlsnr: merge uxsock_trigger() into state machine

[Martin Wilck]

On Wed, 2021-09-15 at 22:32 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 10, 2021 at 01:41:15PM +0200, mwilck at suse.com wrote:
> > From: Martin Wilck <mwilck at suse.com>
> > 
> > This patch sets up the bulk of the state machine. The idea is to
> > fall through the case labels as long as possible (when steps
> > succeed)
> > and return to the caller if either an error occurs, or it becomes
> > necessary to wait for some pollable condition.
> > 
> > While doing this, switch to negative error codes for the functions
> > in uxlsnr.c (e.g. parse_cmd()). Positive return codes are reserved
> > for the cli_handler functions themselves. This way we can clearly
> > distinguish the error source, and avoid confusion and misleading
> > error messages. No cli_handler returns negative values.
> > 
> > Note: with this patch applied, clients may hang and time out if
> > the handler fails to acquire the vecs lock. This will be fixed in
> > the
> > follow-up patch "multipathd: uxlsnr: add idle notification".
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  multipathd/uxlsnr.c | 145 ++++++++++++++++++++++++----------------
> > ----
> >  1 file changed, 80 insertions(+), 65 deletions(-)
> > 
> > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > index ff9604f..553274b 100644
> > --- a/multipathd/uxlsnr.c
> > +++ b/multipathd/uxlsnr.c
> >  
> > -static int uxsock_trigger(struct client *c, void *trigger_data)
> > +void default_reply(struct client *c, int r)
> >  {
> > -       struct vectors * vecs;
> > -       int r = 1;
> > -
> > -       vecs = (struct vectors *)trigger_data;
> > -
> > -       r = parse_cmd(c);
> > -
> > -       if (r == 0 && c->cmdvec && VECTOR_SIZE(c->cmdvec) > 0) {
> > -               struct key *kw = VECTOR_SLOT(c->cmdvec, 0);
> > -
> > -               if (!c->is_root && kw->code != LIST)
> > -                       r = EPERM;
> > -       }
> > -
> > -       if (r == 0 && c->handler)
> > -               r = execute_handler(c, vecs, uxsock_timeout /
> > 1000);
> > -
> > -       if (c->cmdvec) {
> > -               free_keys(c->cmdvec);
> > -               c->cmdvec = NULL;
> > -       }
> > -
> > -       if (r > 0) {
> > -               switch(r) {
> > -               case ETIMEDOUT:
> > -                       append_strbuf_str(&c->reply, "timeout\n");
> > -                       break;
> > -               case EPERM:
> > -                       append_strbuf_str(&c->reply,
> > -                                         "permission deny: need to
> > be root\n");
> > -                       break;
> > -               default:
> > -                       append_strbuf_str(&c->reply, "fail\n");
> > -                       break;
> > -               }
> > -       }
> > -       else if (!r && get_strbuf_len(&c->reply) == 0) {
> > +       switch(r) {
> > +       case -EINVAL:
> > +       case -ESRCH:
> > +       case -ENOMEM:
> 
> get_cmdvec() returns positive errors and do_genhelp() expects
> positive
> errors, but this expects negative errors.

parse_cmd() already negates the return value of get_cmdvec().
But you're right wrt do_genhelp(). I'll fix it.

(FTR, when I worked on the patch series, I considered fixing up the
return codes of all cli_handler functions, but I gave up for now, it
was too much work for no real benefit).

Regards
Martin