blockdev applet proposal

[Denys Vlasenko]

On Sun, Sep 5, 2010 at 4:14 PM, Sergey Naumov <sknaumov at gmail.com> wrote:
> performed with sysctl applet, but, as I understand, it couldn't. So I
> wrote little blockdev applet and I want to ask whether I can
> contribute to busybox with it?

Sure!

> Standard blockdev (from util-linux-ng package) has -q -v and -V for
> quiet, verbose and Version. This flags are removed.
> Also I removed --report command (with output similar to "fdisk -l") -
> I think it is not so nessesary at an embedded system,
> --setra, --getra, --setfra, --getfra - I do not know the goal of these
> flags so I cant test it properly => they are removed. I think they
> could be easily added if someone explains their purpose and use cases.
> Standard blockdev allows to perform multiple commands on multiple
> devices by blockdev --cmd1 --cmd2 ... --cmdM dev1 ... devN, but I have
> never used this feature before so I think that it is not so nessesary.
> Only "blockdev --cmd [CMDARG] dev" format supported.

This looks ok.

> Code of applet is exported from original blockdev and then slightly reworked:
> 1. I have deleted some fields from structure for ioctl variants, so
> static array of structures is slightly smaller.
> 2. There was some ioctl variants in this array which absent in the
> manual and couldn't be executed => they were deleted.
> 2. Help is moved from source to usage.src.h.

Please put all things (help/config/applet/kbuild) into one file.
See procps/pmap.c for an example how to do it.

Code review:

struct bdc bdcms[] = {...}   - should be "*const* struct bdc bdcms[] = {...}"

struct bdc->argtype and ->flags can be uint8_t's, ->argval can be int8_t.
->ioc can be uint16_t (IIRC all ioctl constants fin into 16 bits).
IOCTL_ENTRY macro is not needed.

Please move blockdev_main() to the end of the file, you won't need
forward references that way.


if (-1 == (j = find_cmd(argv[1]))) return ERR_NOCMD;

For better readability, the above should look like this:

j = find_cmd(argv[1]);
if (-1 == j)
        return ERR_NOCMD;


+       switch(bdcms[j].argtype) {
+       default:
+       case ARG_NONE:

I bet it will generate a few less bytes if you omit "case ARG_NONE":

       switch (bdcms[j].argtype) {
       default: /* ARG_NONE */



iarg = atoi(argv[2]);

Use xatoi[_positive](), it will catch bad input.


What's the point in returning ERR_IOCTL from do_cmd() to main?
You can instead simply call bb_perror_msg_and_die("ioctl failure")
in do_cmd(). Same applies to some (all?) other ERR_foo constants.


Style: "switch(" should be "switch (" and there should not be
trailing whitespace.



> By the way, I cant delete busybox.html with doc-clean along with
> usage.h, applets.h and NUM_applets.h with clean and distclean. Is it
> correct?

No, it seems to be a bug. I don't use "make clean", so it accumulates
bugs over time...
-- 
vda