Re: FYI, more id.c changes

[Jim Meyering]

Jim Meyering wrote:

> Jim Meyering wrote:
>> While investigating today's bug, I noticed that a plain old "id -G"
>> would call getcon unnecessarily.  It's not going to print a context
>> string, so it obviously doesn't need to call getcon.
>>
>> While addressing that, factoring and cleaning up, I noticed this:
>>
>>     Old behavior: nonsensical diagnostic, since with -Z,
>>     you don't get the default format:
>>
>>         $ id -Z -n
>>         id: cannot print only names or real IDs in default format
>>
>>     New: -n is ignored with --context (-Z)
>>
>>         $ src/id -Z -n
>>         unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>
>> Considering it's at least two separate issues, I will separate this
>> into two (or more) patches, with at least one more test:
>
> Just as well that I separated them.
> In doing so, I found an additional "argc - optind" to factor out.
> I haven't added a test for the second or third patches -- out of time,
> and they don't seem worth it.  However, if someone else would like to,
> you're welcome.
>
>>From a6719d9f7252bbd85eaab840a61dfc93d57b05c5 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Fri, 27 Apr 2012 20:44:58 +0200
> Subject: [PATCH 1/3] maint: id: minor factorization
>
> * src/id.c (main): Factor out uses of "argc - optind".
> Move option-consistency checks to precede the potential getcon call.
> ---
>  src/id.c |   23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/src/id.c b/src/id.c
> index c600e63..e1b51e7 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -163,34 +163,35 @@ main (int argc, char **argv)
>          }
>      }
>
> -  if (1 < argc - optind)
> +  size_t n_ids = argc - optind;
> +  if (1 < n_ids)
>      {
>        error (0, 0, _("extra operand %s"), quote (argv[optind + 1]));
>        usage (EXIT_FAILURE);
>      }
>
> -  if (argc - optind == 1 && just_context)
> +  if (n_ids && just_context)
>      error (EXIT_FAILURE, 0,
>             _("cannot print security context when user specified"));
>
> +  if (just_user + just_group + just_group_list + just_context > 1)
> +    error (EXIT_FAILURE, 0, _("cannot print \"only\" of more than one 
> choice"));
> +
> +  if (just_user + just_group + just_group_list == 0 && (use_real || 
> use_name))
> +    error (EXIT_FAILURE, 0,
> +           _("cannot print only names or real IDs in default format"));
> +
>    /* If we are on a selinux-enabled kernel and no user is specified,
>       get our context. Otherwise, leave the context variable alone -
>       it has been initialized known invalid value and will be not
>       displayed in print_full_info() */
> -  if (selinux_enabled && argc == optind)
> +  if (selinux_enabled && n_ids == 0)
>      {
>        if (getcon (&context) && just_context)
>          error (EXIT_FAILURE, 0, _("can't get process context"));
>      }
>
> -  if (just_user + just_group + just_group_list + just_context > 1)
> -    error (EXIT_FAILURE, 0, _("cannot print \"only\" of more than one 
> choice"));
> -
> -  if (just_user + just_group + just_group_list == 0 && (use_real || 
> use_name))
> -    error (EXIT_FAILURE, 0,
> -           _("cannot print only names or real IDs in default format"));
> -
> -  if (argc - optind == 1)
> +  if (n_ids == 1)
>      {
>        struct passwd *pwd = getpwnam (argv[optind]);
>        if (pwd == NULL)
> --
> 1.7.10.365.g7cacb
>
>
>>From fbbb4e0e351f528dc662064b413bcd76c44767fc Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Fri, 27 Apr 2012 21:24:03 +0200
> Subject: [PATCH 2/3] id: don't call getcon unnecessarily
>
> * src/id.c (main): Invocations like "id" and "id -G" would call getcon
> to determine the current security context even though that result would
> not be used.  Similarly, when POSIXLY_CORRECT is set.  Rearrange
> conditionals and hoist the POSIXLY_CORRECT test so that we call
> getcon only when necessary.
> ---
>  src/id.c |   21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/src/id.c b/src/id.c
> index e1b51e7..058622c 100644
> --- a/src/id.c
> +++ b/src/id.c
> @@ -177,16 +177,23 @@ main (int argc, char **argv)
>    if (just_user + just_group + just_group_list + just_context > 1)
>      error (EXIT_FAILURE, 0, _("cannot print \"only\" of more than one 
> choice"));
>
> -  if (just_user + just_group + just_group_list == 0 && (use_real || 
> use_name))
> +  if (default_format && (use_real || use_name))
>      error (EXIT_FAILURE, 0,
>             _("cannot print only names or real IDs in default format"));
>
> -  /* If we are on a selinux-enabled kernel and no user is specified,
> -     get our context. Otherwise, leave the context variable alone -
> -     it has been initialized known invalid value and will be not
> -     displayed in print_full_info() */
> -  if (selinux_enabled && n_ids == 0)
> +  bool default_format = (just_user + just_group + just_group_list == 0);

Well, don't waste time looking at those.
In a last-minute rebase, I didn't notice that while
resolving a conflict I moved the declaration of default_format
to be after the first use.  Oops.  Adjusting...