| Message ID | a7e67594e748d1b91f755dd971f222afa09f5443.1707513012.git.mjt@tls.msk.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: refersh options and --help handling | expand |
On Sat, Feb 10, 2024 at 12:22:44AM +0300, Michael Tokarev wrote: > also add short description to each command and use it in --help > > Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> > --- > qemu-img.c | 42 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/qemu-img.c b/qemu-img.c > index d9c5c6078b..e57076738e 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -61,6 +61,7 @@ > typedef struct img_cmd_t { > const char *name; > int (*handler)(const struct img_cmd_t *ccmd, int argc, char **argv); > + const char *description; > } img_cmd_t; > > enum { > @@ -130,11 +131,12 @@ static G_NORETURN > void cmd_help(const img_cmd_t *ccmd, > const char *syntax, const char *arguments) > { > - printf("qemu-img %s %s" > + printf("qemu-img %s: %s. Usage:\n" > + "qemu-img %s %s" This ends up looking a bit muddled together. I don't think we need repeat 'qemu-img <cmd>' twice, and could add a little more whitespace eg instead of: $ ./build/qemu-img check --help qemu-img check: Check basic image integrity. Usage: qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] [--output human|json] [--object OBJDEF] FILENAME Arguments: ...snip... have it look like $ ./build/qemu-img check --help Check basic image integrity. Usage: qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] [--output human|json] [--object OBJDEF] FILENAME Arguments: ...snip... > "Arguments:\n" > " -h|--help - print this help and exit\n" > "%s", > - ccmd->name, syntax, arguments); > + ccmd->name, ccmd->description, ccmd->name, syntax, arguments); > exit(EXIT_SUCCESS); > } > > @@ -5746,10 +5748,36 @@ out: > } > > static const img_cmd_t img_cmds[] = { > -#define DEF(option, callback, arg_string) \ > - { option, callback }, > -#include "qemu-img-cmds.h" > -#undef DEF > + { "amend", img_amend, > + "Update format-specific options of the image" }, > + { "bench", img_bench, > + "Run simple image benchmark" }, > + { "bitmap", img_bitmap, > + "Perform modifications of the persistent bitmap in the image" }, > + { "check", img_check, > + "Check basic image integrity" }, > + { "commit", img_commit, > + "Commit image to its backing file" }, > + { "compare", img_compare, > + "Check if two images have the same contents" }, > + { "convert", img_convert, > + "Copy one image to another with optional format conversion" }, > + { "create", img_create, > + "Create and format new image file" }, > + { "dd", img_dd, > + "Copy input to output with optional format conversion" }, > + { "info", img_info, > + "Display information about image" }, > + { "map", img_map, > + "Dump image metadata" }, > + { "measure", img_measure, > + "Calculate file size requred for a new image" }, > + { "rebase", img_rebase, > + "Change backing file of the image" }, > + { "resize", img_resize, > + "Resize the image to the new size" }, > + { "snapshot", img_snapshot, > + "List or manipulate snapshots within image" }, > { NULL, NULL, }, > }; > > @@ -5813,7 +5841,7 @@ QEMU_IMG_VERSION > " [[enable=]<pattern>][,events=<file>][,file=<file>]\n" > "Recognized commands (run qemu-img command --help for command-specific help):\n"); > for (cmd = img_cmds; cmd->name != NULL; cmd++) { > - printf(" %s\n", cmd->name); > + printf(" %s - %s\n", cmd->name, cmd->description); > } > c = printf("Supported image formats:"); > bdrv_iterate_format(format_print, &c, false); > -- > 2.39.2 > > With regards, Daniel
20.02.2024 21:48, Daniel P. Berrangé: ... > $ ./build/qemu-img check --help > Check basic image integrity. > > Usage: > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > [--output human|json] [--object OBJDEF] FILENAME > > Arguments: $ ./build/qemu-img check --help Check basic image integrity. Usage: qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] [--output human|json] [--object OBJDEF] FILENAME Arguments: ... Or just: Check basic image integrity: qemu-img check... In all cases I tried to make the whole thing as compact as possible, to (almost) fit on a standard terminal. The extra empty lines between different arguments makes it almost impossible. I think if indentation will be larger, it will be easier to read. Let me experiment a bit.. >> "Arguments:\n" >> " -h|--help - print this help and exit\n" btw, the common way is to use comma here, not "|", -- -h,--help - ... Again, I especially omitted space after "|" to make it more compact. Maybe for no good. We've really big amount of options here, conflicting and illogical in some cases, which's been added without much thinking. All this makes me think it will be difficult to automate generation of all this text for both code and docs.. Thanks! /mjt
On Tue, Feb 20, 2024 at 10:02:32PM +0300, Michael Tokarev wrote: > 20.02.2024 21:48, Daniel P. Berrangé: > ... > > $ ./build/qemu-img check --help > > Check basic image integrity. > > > > Usage: > > > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > > [--output human|json] [--object OBJDEF] FILENAME > > > > Arguments: > > $ ./build/qemu-img check --help > Check basic image integrity. Usage: > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > [--output human|json] [--object OBJDEF] FILENAME > > Arguments: > ... > > Or just: > > Check basic image integrity: > > qemu-img check... > > > In all cases I tried to make the whole thing as compact as possible, > to (almost) fit on a standard terminal. The extra empty lines between > different arguments makes it almost impossible. IMHO fitting on a "standard" terminal is OK in terms of width, but should be a non-goal in terms of height. Readability is more important than avoiding vertical scroll. > > > "Arguments:\n" > > > " -h|--help - print this help and exit\n" > > btw, the common way is to use comma here, not "|", -- > -h,--help - ... > > Again, I especially omitted space after "|" to make it > more compact. Maybe for no good. Yes, a comma with a space would look nicer. If we have the description on the following line, then there's no width limit problems there. With regards, Daniel
20.02.2024 21:48, Daniel P. Berrangé: > This ends up looking a bit muddled together. I don't think we > need repeat 'qemu-img <cmd>' twice, and could add a little > more whitespace > > eg instead of: > > $ ./build/qemu-img check --help > qemu-img check: Check basic image integrity. Usage: > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > [--output human|json] [--object OBJDEF] FILENAME > Arguments: > ...snip... > > have it look like > > $ ./build/qemu-img check --help > Check basic image integrity. > > Usage: > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > [--output human|json] [--object OBJDEF] FILENAME > > Arguments: > ...snip... Here's the current way how `create' help text looks like: $ ./qemu-img create --help Create and format qemu image file. Usage: qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]] [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]] Arguments: -h, --help print this help and exit -q, --quiet quiet operations -f, --format FMT specifies format of the new image, default is raw -o, --options FMT_OPTS format-specific options ('-o list' for list) -b, --backing BACKING_FILENAME stack new image on top of BACKING_FILENAME (for formats which support stacking) -F, --backing-format BACKING_FMT specify format of BACKING_FILENAME -u, --backing-unsafe do not fail if BACKING_FMT can not be read --object OBJDEF QEMU user-creatable object (eg encryption key) FILENAME image file to create. It will be overridden if exists SIZE image size with optional suffix (multiplies in 1024) SIZE is required unless BACKING_IMG is specified, in which case it will be the same as size of BACKING_IMG Maybe it's a good idea to add newlines around the "syntax" part, ie, after "Usage:" and before "Arguments:". I don't think it needs extra newlines between each argument description though, - this way it becomes just too long. What do you think? Thanks, /mjt
On Wed, Feb 21, 2024 at 07:31:42PM +0300, Michael Tokarev wrote: > 20.02.2024 21:48, Daniel P. Berrangé: > > > This ends up looking a bit muddled together. I don't think we > > need repeat 'qemu-img <cmd>' twice, and could add a little > > more whitespace > > > > eg instead of: > > > > $ ./build/qemu-img check --help > > qemu-img check: Check basic image integrity. Usage: > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > > [--output human|json] [--object OBJDEF] FILENAME > > Arguments: > > ...snip... > > > > have it look like > > > > $ ./build/qemu-img check --help > > Check basic image integrity. > > > > Usage: > > > > qemu-img check [-f FMT | --image-opts] [-T CACHE_MODE] [-r] [-u] > > [--output human|json] [--object OBJDEF] FILENAME > > > > Arguments: > > ...snip... > > Here's the current way how `create' help text looks like: > > $ ./qemu-img create --help > Create and format qemu image file. Usage: > qemu-img create [-f FMT] [-o FMT_OPTS] [-b BACKING_FILENAME [-F BACKING_FMT]] > [--object OBJDEF] [-u] FILENAME [SIZE[bkKMGTPE]] > Arguments: > -h, --help > print this help and exit > -q, --quiet > quiet operations > -f, --format FMT > specifies format of the new image, default is raw > -o, --options FMT_OPTS > format-specific options ('-o list' for list) > -b, --backing BACKING_FILENAME > stack new image on top of BACKING_FILENAME > (for formats which support stacking) > -F, --backing-format BACKING_FMT > specify format of BACKING_FILENAME > -u, --backing-unsafe > do not fail if BACKING_FMT can not be read > --object OBJDEF > QEMU user-creatable object (eg encryption key) > FILENAME > image file to create. It will be overridden if exists > SIZE > image size with optional suffix (multiplies in 1024) > SIZE is required unless BACKING_IMG is specified, > in which case it will be the same as size of BACKING_IMG > > Maybe it's a good idea to add newlines around the "syntax" part, > ie, after "Usage:" and before "Arguments:". I don't think it needs > extra newlines between each argument description though, - this way > it becomes just too long. > > What do you think? I still prefer to have more vertical whitespace, as I find it harder to read through without it. I was using the typical man page option / usage formatting as a guide in my feedback. Still, it would be useful to see what other maintainers think, as I'm just one data point. With regards, Daniel
diff --git a/qemu-img.c b/qemu-img.c index d9c5c6078b..e57076738e 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -61,6 +61,7 @@ typedef struct img_cmd_t { const char *name; int (*handler)(const struct img_cmd_t *ccmd, int argc, char **argv); + const char *description; } img_cmd_t; enum { @@ -130,11 +131,12 @@ static G_NORETURN void cmd_help(const img_cmd_t *ccmd, const char *syntax, const char *arguments) { - printf("qemu-img %s %s" + printf("qemu-img %s: %s. Usage:\n" + "qemu-img %s %s" "Arguments:\n" " -h|--help - print this help and exit\n" "%s", - ccmd->name, syntax, arguments); + ccmd->name, ccmd->description, ccmd->name, syntax, arguments); exit(EXIT_SUCCESS); } @@ -5746,10 +5748,36 @@ out: } static const img_cmd_t img_cmds[] = { -#define DEF(option, callback, arg_string) \ - { option, callback }, -#include "qemu-img-cmds.h" -#undef DEF + { "amend", img_amend, + "Update format-specific options of the image" }, + { "bench", img_bench, + "Run simple image benchmark" }, + { "bitmap", img_bitmap, + "Perform modifications of the persistent bitmap in the image" }, + { "check", img_check, + "Check basic image integrity" }, + { "commit", img_commit, + "Commit image to its backing file" }, + { "compare", img_compare, + "Check if two images have the same contents" }, + { "convert", img_convert, + "Copy one image to another with optional format conversion" }, + { "create", img_create, + "Create and format new image file" }, + { "dd", img_dd, + "Copy input to output with optional format conversion" }, + { "info", img_info, + "Display information about image" }, + { "map", img_map, + "Dump image metadata" }, + { "measure", img_measure, + "Calculate file size requred for a new image" }, + { "rebase", img_rebase, + "Change backing file of the image" }, + { "resize", img_resize, + "Resize the image to the new size" }, + { "snapshot", img_snapshot, + "List or manipulate snapshots within image" }, { NULL, NULL, }, }; @@ -5813,7 +5841,7 @@ QEMU_IMG_VERSION " [[enable=]<pattern>][,events=<file>][,file=<file>]\n" "Recognized commands (run qemu-img command --help for command-specific help):\n"); for (cmd = img_cmds; cmd->name != NULL; cmd++) { - printf(" %s\n", cmd->name); + printf(" %s - %s\n", cmd->name, cmd->description); } c = printf("Supported image formats:"); bdrv_iterate_format(format_print, &c, false);
also add short description to each command and use it in --help Signed-off-by: Michael Tokarev <mjt@tls.msk.ru> --- qemu-img.c | 42 +++++++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-)