3:21 a.m.
On 4/14/23 10:11, Richard W.M. Jones wrote:
On Fri, Apr 14, 2023 at 09:59:53AM +0200, Laszlo Ersek wrote:
> Two of the enum constants that denote command line options are
> inconsistently named with the rest: all identifiers should be
> <purpose>_OPTION, but LONG_OPTIONS and SHORT_OPTIONS (which are supposed
> to list the long and short options) don't conform. Rename them.
>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516
> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com>
> ---
> copy/main.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/copy/main.c b/copy/main.c
> index f9a714c4f677..bb67e97ff97a 100644
> --- a/copy/main.c
> +++ b/copy/main.c
> @@ -107,8 +107,8 @@ main (int argc, char *argv[])
> {
> enum {
> HELP_OPTION = CHAR_MAX + 1,
> - LONG_OPTIONS,
> - SHORT_OPTIONS,
> + LONG_OPTIONS_OPTION,
> + SHORT_OPTIONS_OPTION,
> ALLOCATED_OPTION,
> DESTINATION_IS_ZERO_OPTION,
> FLUSH_OPTION,
> @@ -120,7 +120,7 @@ main (int argc, char *argv[])
> const char *short_options = "C:pR:S:T:vV";
> const struct option long_options[] = {
> { "help", no_argument, NULL, HELP_OPTION },
> - { "long-options", no_argument, NULL, LONG_OPTIONS },
> + { "long-options", no_argument, NULL, LONG_OPTIONS_OPTION
},
> { "allocated", no_argument, NULL, ALLOCATED_OPTION },
> { "connections", required_argument, NULL, 'C' },
> { "destination-is-zero",no_argument, NULL,
DESTINATION_IS_ZERO_OPTION },
> @@ -130,7 +130,7 @@ main (int argc, char *argv[])
> { "queue-size", required_argument, NULL, QUEUE_SIZE_OPTION },
> { "request-size", required_argument, NULL, REQUEST_SIZE_OPTION
},
> { "requests", required_argument, NULL, 'R' },
> - { "short-options", no_argument, NULL, SHORT_OPTIONS },
> + { "short-options", no_argument, NULL, SHORT_OPTIONS_OPTION
},
> { "sparse", required_argument, NULL, 'S' },
> { "synchronous", no_argument, NULL, SYNCHRONOUS_OPTION
},
> { "target-is-zero", no_argument, NULL,
DESTINATION_IS_ZERO_OPTION },
> @@ -155,7 +155,7 @@ main (int argc, char *argv[])
> case HELP_OPTION:
> usage (stdout, EXIT_SUCCESS);
>
> - case LONG_OPTIONS:
> + case LONG_OPTIONS_OPTION:
> for (i = 0; long_options[i].name != NULL; ++i) {
> if (strcmp (long_options[i].name, "long-options") != 0 &&
> strcmp (long_options[i].name, "short-options") != 0)
> @@ -163,7 +163,7 @@ main (int argc, char *argv[])
> }
> exit (EXIT_SUCCESS);
>
> - case SHORT_OPTIONS:
> + case SHORT_OPTIONS_OPTION:
> for (i = 0; short_options[i]; ++i) {
> if (short_options[i] != ':' && short_options[i] !=
'+')
> printf ("-%c\n", short_options[i]);
Assuming (from the cover message) that the same will be done later for
nbdinfo, fuse etc then ACK.
Hmm. I didn't intend to do that.
It seems that the following further files use the _OPTION suffix:
dump/dump.c
fuse/nbdfuse.c
info/main.c
ublk/nbdublk.c
From these, "dump/dump.c" and "info/main.c"
don't exceed 80 chars, so I
didn't plan to modify them at all.
"fuse/nbdfuse.c" and "ublk/nbdublk.c"
need wrapping, but not in their option tables.
Renaming the options in these four other files, just for consistency's
sake, seems overkill. Instead, I think we should pick a different
approach for "copy/main.c".
In the strictest sense, I only need to shorten
DESTINATION_IS_ZERO_OPTION by three characters. Should I rename it to
DEST_IS_ZERO_OPTION, or TARGET_IS_ZERO_OPTION? (The latter is better,
because we already have a "--target-is-zero" long option, aliasing
"--destination-is-zero".)
So I'd replace patches #1 and #2 with that, and keep #3 and #4.
Laszlo
I was going to say that we use LONG_OPTIONS & SHORT_OPTIONS elsewhere,
but actually we don't. We use --long-options and --short-options
extensively (eg. in guestfs-tools) but those are implemented
differently.
Rich.