| Message ID | 20240927061121.573271-6-mjt@tls.msk.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: refersh options and --help handling, cleanups | expand |
Am 27.09.2024 um 08:10 hat Michael Tokarev geschrieben: > Create helper function cmd_help() to display command-specific > help text, and use it to print --help for 'create' subcommand. > > Add missing long options (eg --format) in img_create(). > > Remove usage of missing_argument()/unrecognized_option() in > img_create(). > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > qemu-img.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 61 insertions(+), 8 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index e8234104e5..7ed5e6d1a8 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -132,6 +132,32 @@ void unrecognized_option(const char *option) > error_exit("qemu-img", "unrecognized option '%s'", option); > } > > +/* > + * Print --help output for a command and exit. > + * syntax and description are multi-line with trailing EOL > + * (to allow easy extending of the text) > + * syntax has each subsequent line indented by 8 chars. > + * desrciption is indented by 2 chars for argument on each own line, s/desrciption/description/ I would also suggest writing @syntax and @description as we do in other places to indicate that we're talking about parameter names here rather than normal English words. > + * and with 5 chars for argument description (like -h arg below). > + */ > +static G_NORETURN > +void cmd_help(const img_cmd_t *ccmd, > + const char *syntax, const char *arguments) > +{ > + printf( > +"Usage:\n" > +"\n" > +" %s %s %s" > +"\n" > +"Arguments:\n" > +" -h, --help\n" > +" print this help and exit\n" > +"%s\n", > + "qemu-img", ccmd->name, > + syntax, arguments); The last two lines should easily fit on a single one. > + exit(EXIT_SUCCESS); > +} > + > /* Please keep in synch with docs/tools/qemu-img.rst */ > static G_NORETURN > void help(void) > @@ -530,23 +556,48 @@ static int img_create(const img_cmd_t *ccmd, int argc, char **argv) > for(;;) { > static const struct option long_options[] = { > {"help", no_argument, 0, 'h'}, > + {"quiet", no_argument, 0, 'q'}, > {"object", required_argument, 0, OPTION_OBJECT}, > + {"format", required_argument, 0, 'f'}, > + {"backing", required_argument, 0, 'b'}, > + {"backing-format", required_argument, 0, 'F'}, > + {"backing-unsafe", no_argument, 0, 'u'}, > + {"options", required_argument, 0, 'o'}, > {0, 0, 0, 0} > }; > - c = getopt_long(argc, argv, ":F:b:f:ho:qu", > + c = getopt_long(argc, argv, "F:b:f:ho:qu", > long_options, NULL); Can we please keep the same order in long_options and in the options string? > if (c == -1) { > break; > } > switch(c) { > - case ':': > - missing_argument(argv[optind - 1]); > - break; > - case '?': > - unrecognized_option(argv[optind - 1]); > - break; > case 'h': > - help(); > + cmd_help(ccmd, Is this new help text copied from somewhere or is it written from scratch? I couldn't find the source if it was copied, so I'll treat it as new and compare it only with the generic help text and the man page. > +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n" > +" [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n" This is yet another order. It also seems to miss -q. The existing documentation has this: create [--object OBJECTDEF] [-q] [-f FMT] [-b BACKING_FILE [-F BACKING_FMT]] [-u] [-o OPTIONS] FILENAME [SIZE] Which is a fourth different order and description. It would probably be good to consolidate on a single option and way to name parameter placeholders. > +, > +" -q, --quiet\n" > +" quiet operations\n" > +" -f, --format FMT\n" > +" specifies format of the new image, default is raw\n" s/format/the format/ I would suggest "(default: raw)" as the format to use for defaults. It's a bit easier to read when it's clearly not part of the sentence. > +" -o, --options FMT_OPTS\n" > +" format-specific options ('-o list' for list)\n" Do you mean '-o help'? > +" -b, --backing BACKING_FILENAME\n" > +" stack new image on top of BACKING_FILENAME\n" > +" (for formats which support stacking)\n" "stacking" isn't terminology we use elsewhere. Maybe "create image as a copy-on-write overlay on top of BACKING_FILENAME (not supported by all formats)"? Not sure if it's worth having the parenthesis at all, many options of qemu-img will only work with some formats. > +" -F, --backing-format BACKING_FMT\n" > +" specify format of BACKING_FILENAME\n" -f said "specifies" (which sounds a bit better to me), let's stay consistent. Same s/format/the format/, too. > +" -u, --backing-unsafe\n" > +" do not fail if BACKING_FMT can not be read\n" BACKING_FILENAME? Maybe "don't try to open BACKING_FILENAME for verification"? > +" --object OBJDEF\n" > +" QEMU user-creatable object (eg encryption key)\n" s/eg/e.g./ Most other options start with a verb. Maybe "define a user-creatable object"? > +" FILENAME\n" > +" image file to create. It will be overridden if exists\n" overwritten? If you have one full stop in it, I think we should have one at the end, too: "...if it exists.\n" > +" SIZE\n" > +" image size with optional suffix (multiplies in 1024)\n" Powers of 1024? The existing help is a bit more explicit about the supported suffixes: " 'size' is the disk image size in bytes. Optional suffixes\n" " 'k' or 'K' (kilobyte, 1024), 'M' (megabyte, 1024k), 'G' (gigabyte, 1024M),\n" " 'T' (terabyte, 1024G), 'P' (petabyte, 1024T) and 'E' (exabyte, 1024P) are\n" " supported. 'b' is ignored.\n" > +" SIZE is required unless BACKING_IMG is specified,\n" > +" in which case it will be the same as size of BACKING_IMG\n" "it will default to the same size as BACKING_IMG"? > +); > break; > case 'F': > base_fmt = optarg; > @@ -571,6 +622,8 @@ static int img_create(const img_cmd_t *ccmd, int argc, char **argv) > case OPTION_OBJECT: > user_creatable_process_cmdline(optarg); > break; > + default: > + tryhelp(argv[0]); > } > } Kevin
diff --git a/qemu-img.c b/qemu-img.c index e8234104e5..7ed5e6d1a8 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -132,6 +132,32 @@ void unrecognized_option(const char *option) error_exit("qemu-img", "unrecognized option '%s'", option); } +/* + * Print --help output for a command and exit. + * syntax and description are multi-line with trailing EOL + * (to allow easy extending of the text) + * syntax has each subsequent line indented by 8 chars. + * desrciption is indented by 2 chars for argument on each own line, + * and with 5 chars for argument description (like -h arg below). + */ +static G_NORETURN +void cmd_help(const img_cmd_t *ccmd, + const char *syntax, const char *arguments) +{ + printf( +"Usage:\n" +"\n" +" %s %s %s" +"\n" +"Arguments:\n" +" -h, --help\n" +" print this help and exit\n" +"%s\n", + "qemu-img", ccmd->name, + syntax, arguments); + exit(EXIT_SUCCESS); +} + /* Please keep in synch with docs/tools/qemu-img.rst */ static G_NORETURN void help(void) @@ -530,23 +556,48 @@ static int img_create(const img_cmd_t *ccmd, int argc, char **argv) for(;;) { static const struct option long_options[] = { {"help", no_argument, 0, 'h'}, + {"quiet", no_argument, 0, 'q'}, {"object", required_argument, 0, OPTION_OBJECT}, + {"format", required_argument, 0, 'f'}, + {"backing", required_argument, 0, 'b'}, + {"backing-format", required_argument, 0, 'F'}, + {"backing-unsafe", no_argument, 0, 'u'}, + {"options", required_argument, 0, 'o'}, {0, 0, 0, 0} }; - c = getopt_long(argc, argv, ":F:b:f:ho:qu", + c = getopt_long(argc, argv, "F:b:f:ho:qu", long_options, NULL); if (c == -1) { break; } switch(c) { - case ':': - missing_argument(argv[optind - 1]); - break; - case '?': - unrecognized_option(argv[optind - 1]); - break; case 'h': - help(); + cmd_help(ccmd, +"[-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]]\n" +" [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]]\n" +, +" -q, --quiet\n" +" quiet operations\n" +" -f, --format FMT\n" +" specifies format of the new image, default is raw\n" +" -o, --options FMT_OPTS\n" +" format-specific options ('-o list' for list)\n" +" -b, --backing BACKING_FILENAME\n" +" stack new image on top of BACKING_FILENAME\n" +" (for formats which support stacking)\n" +" -F, --backing-format BACKING_FMT\n" +" specify format of BACKING_FILENAME\n" +" -u, --backing-unsafe\n" +" do not fail if BACKING_FMT can not be read\n" +" --object OBJDEF\n" +" QEMU user-creatable object (eg encryption key)\n" +" FILENAME\n" +" image file to create. It will be overridden if exists\n" +" SIZE\n" +" image size with optional suffix (multiplies in 1024)\n" +" SIZE is required unless BACKING_IMG is specified,\n" +" in which case it will be the same as size of BACKING_IMG\n" +); break; case 'F': base_fmt = optarg; @@ -571,6 +622,8 @@ static int img_create(const img_cmd_t *ccmd, int argc, char **argv) case OPTION_OBJECT: user_creatable_process_cmdline(optarg); break; + default: + tryhelp(argv[0]); } }
Create helper function cmd_help() to display command-specific help text, and use it to print --help for 'create' subcommand. Add missing long options (eg --format) in img_create(). Remove usage of missing_argument()/unrecognized_option() in img_create(). Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- qemu-img.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 61 insertions(+), 8 deletions(-)