[sheepdog] [PATCH] dog/node: remove checking argument in md plug/unplug since root cause is found

[Hitoshi Mitake]

At Thu, 07 Aug 2014 14:26:28 +0800,
Ruoyu wrote:
> 
> 
> On 2014年08月07日 13:39, Hitoshi Mitake wrote:
> > At Thu,  7 Aug 2014 12:08:23 +0800,
> > Ruoyu wrote:
> >> The purpose of the checking in the function is to ensure the
> >> argument is not null. But it is not needed any longer because
> >> the argument should be guaranteed in the framework as long as
> >> CMD_NEED_ARG is specified in subcommand.
> >>
> >> Since the problem is solved in commit e6d1706, we can remove
> >> the checking safely.
> >>
> >> Signed-off-by: Ruoyu <liangry at ucweb.com>
> >> ---
> >>   dog/node.c | 8 --------
> >>   1 file changed, 8 deletions(-)
> >>
> >> diff --git a/dog/node.c b/dog/node.c
> >> index b6fd929..f9f7351 100644
> >> --- a/dog/node.c
> >> +++ b/dog/node.c
> >> @@ -441,14 +441,6 @@ static int do_plug_unplug(char *disks, bool plug)
> >>   	struct sd_rsp *rsp = (struct sd_rsp *)&hdr;
> >>   	int ret;
> >>   
> >> -	if (!disks)
> >> -		return EXIT_USAGE;
> >> -
> > I agree with that this check isn't required.
> >
> >> -	if (!strlen(disks)) {
> >> -		sd_err("Empty path isn't allowed");
> >> -		return EXIT_FAILURE;
> >> -	}
> > But I think this check is needed. If we execute a command like this:
> > dog node md plug "", sheep dies because sheep daemon assumes a path
> > name passed by dog process is correct.
> >
> > If you agree with this opinion, I can apply this patch with
> > eliminating the deletion of second check. How do you think?
> I think the above checking should be processed in framework, too. 
> Otherwise, we must check it in every function that need arguments.
> 
> I suggest in do_generic_subcommand:
> 
> optind++;
> - if (flags & CMD_NEED_ARG && argc == optind)
> + if (flags & CMD_NEED_ARG && (argc == optind || argv[optind][0] == '\0'))
> subcommand_usage(argv[1], argv[2], EXIT_USAGE);
> ret = sub[i].fn(argc, argv);
> 
> How do you think?

The idea looks good to me. If we add commands which allow empty string
as it argument in the future, let's prepare CMD_ALLOW_EMPTY_ARG like
the CMD_NEED_ARG flag. But currently your idea would work well.

Thanks,
Hitoshi