On Thu, Dec 14, 2023 at 06:08:34PM +0100, Martin Wilck wrote:
> On Tue, 2023-12-12 at 18:53 -0500, Benjamin Marzinski wrote:
> > When multipathd interactive commands fail for certain reasons, like
> > timing out or incorrect permissions, they do not reply with "fail\n".
> > Currently, multipath and multipathc expect that a reply other than
> > "fail\n" means success. Instead, prefix all failure replies with
> > "fail\n", and adapt multipath and multipathc to return failure for
> > any
> > reply starting with "fail\n".
> >
> > Signed-off-by: Benjamin Marzinski <[email protected]>
> > ---
> > libdmmp/libdmmp.c | 6 +++---
> > multipath/main.c | 2 +-
> > multipathd/uxclnt.c | 2 +-
> > multipathd/uxlsnr.c | 12 +++++-------
> > 4 files changed, 10 insertions(+), 12 deletions(-)
> >
> > diff --git a/libdmmp/libdmmp.c b/libdmmp/libdmmp.c
> > index 0025e66d..77f8a6a0 100644
> > --- a/libdmmp/libdmmp.c
> > +++ b/libdmmp/libdmmp.c
> > @@ -321,7 +321,7 @@ invoke:
> > }
> > }
> > if ((*output != NULL) &&
> > - (strncmp(*output, "timeout", strlen("timeout")) == 0))
> > + (strncmp(*output, "fail\ntimeout",
> > strlen("fail\ntimeout")) == 0))
> > flag_check_tmo = true;
> >
> > if (flag_check_tmo == true) {
> > @@ -364,8 +364,8 @@ invoke:
> > }
> >
> > if ((*output != NULL) &&
> > - strncmp(*output, "permission deny",
> > - strlen("permission deny")) == 0) {
> > + strncmp(*output, "fail\npermission deny",
> > + strlen("fail\npermission deny")) == 0) {
> > _error(ctx, "Permission deny, need to be root");
> > rc = DMMP_ERR_PERMISSION_DENY;
> > goto out;
> > diff --git a/multipath/main.c b/multipath/main.c
> > index c37befa5..4b399e21 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > @@ -804,7 +804,7 @@ int delegate_to_multipathd(enum mpath_cmds cmd,
> > }
> >
> > if (reply != NULL && *reply != '\0') {
> > - if (strcmp(reply, "fail\n"))
> > + if (strncmp(reply, "fail\n", 5))
> > r = DELEGATE_OK;
> > if (r != NOT_DELEGATED && strcmp(reply, "ok\n"))
> > printf("%s", reply);
>
> Perhaps here we should check if the reply length is > 5, and if yes,
> strip the "fail\n" part. That would make the output consistent with
> what it used to be.
>
>
> > diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c
> > index c3ae5db6..2401199d 100644
> > --- a/multipathd/uxclnt.c
> > +++ b/multipathd/uxclnt.c
> > @@ -31,7 +31,7 @@ static int process_req(int fd, char * inbuf,
> > unsigned int timeout)
> > return 1;
> > } else {
> > printf("%s", reply);
>
> Same here.
Sure.
-Ben
>
> > - ret = (strcmp(reply, "fail\n") == 0);
> > + ret = (strncmp(reply, "fail\n", 5) == 0);
> > free(reply);
> > return ret;
> > }
> > diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> > index 4d6f258c..4df01666 100644
> > --- a/multipathd/uxlsnr.c
> > +++ b/multipathd/uxlsnr.c
> > @@ -392,6 +392,11 @@ static void drain_idle_fd(int fd)
> >
> > void default_reply(struct client *c, int r)
> > {
> > + if (r == 0) {
> > + append_strbuf_str(&c->reply, "ok\n");
> > + return;
> > + }
> > + append_strbuf_str(&c->reply, "fail\n");
> > switch(r) {
> > case -EINVAL:
> > case -ESRCH:
> > @@ -406,13 +411,6 @@ void default_reply(struct client *c, int r)
> > case -ETIMEDOUT:
> > append_strbuf_str(&c->reply, "timeout\n");
> > break;
> > - case 0:
> > - append_strbuf_str(&c->reply, "ok\n");
> > - break;
> > - default:
> > - /* cli_handler functions return 1 on unspecified
> > error */
> > - append_strbuf_str(&c->reply, "fail\n");
> > - break;
> > }
> > }
> >
