On Wed, 2021-09-15 at 22:32 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 10, 2021 at 01:41:15PM +0200, mwilck@xxxxxxxx wrote:
> > From: Martin Wilck <mwilck@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> > 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
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
