Libguestfs - Apr 2023 - [libnbd PATCH 1/4] copy: rename (LONG|SHORT)_OPTIONS to (LONG|SHORT)_OPTIONS_OPTION

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 at 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.
>

On Fri, Apr 14, 2023 at 10:21:31AM +0200, Laszlo Ersek
wrote:
> 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".)
Or even fold it across a line ..?
> So I'd replace patches #1 and #2 with that, and keep #3 and #4.
I think that's better.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org