Re: [PATCH] who: --mesg now respects also group of a TTY

[Jim Meyering]

Kamil Dudka wrote:
> enclosed is a patch making the output of who --mesg more reliable when
> compiled with --with-tty-group.  The issue was discussed before at
>
> https://bugzilla.redhat.com/454261
>
> and later at
>
> http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/14022
>
> Any comments are welcome!

Hi Kamil,

Thanks for finally addressing this.

Actually, more comments (in the code and log) would be welcome, too ;-)
Please add a little explanation before the new function, is_tty_writable
and list one or both of those URLs for context in the commit log.

...
> diff --git a/NEWS b/NEWS
...
> +** New features
> +
> +  who --mesg now respects also group of a TTY when compiled with
> +  --with-tty-group

Saying it "respects the group..." doesn't really communicate what is
changed.  How about saying that it works now in some cases (details?)
where it used to fail?

> diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4
...

It would be helpful to say how to determine the appropriate group name.
Something like "ls -lg /dev/tty" or
    $ stat --format %G /dev/tty
    tty

> +  # configure options --with-tty-group/--without-tty-group
> +  AC_ARG_WITH([tty-group],
> +    AS_HELP_STRING([--with-tty-group[[[=NAME]]]],
> +      [group used by system for TTYs, "tty" when not specified]
> +      [ (default: do not rely on any group used for TTYs)]),
> +    [tty_group_name=$withval],
> +    [tty_group_name=no])
> +
> +  if test "x$tty_group_name" != xno; then
> +    if test "x$tty_group_name" = xyes; then
> +      tty_group_name=tty
> +    fi
> +    AC_MSG_NOTICE([TTY group used by system set to "$tty_group_name"])
> +    AC_DEFINE_UNQUOTED([TTY_GROUP_NAME], ["$tty_group_name"],
> +      [group used by system for TTYs])
> +  fi
>  ])
...
> diff --git a/src/who.c b/src/who.c
...

There is enough duplication below that
I'd prefer to move the #ifdef/#endif into the function.

Also, please humor me and put the "const" after the type name:

  is_tty_writable (struct stat const *pstat)

> +#ifdef TTY_GROUP_NAME
> +static bool
> +is_tty_writable (const struct stat *pstat)
> +{
> +  struct group *ttygr = getgrnam (TTY_GROUP_NAME);
> +  if (!ttygr || (pstat->st_gid != ttygr->gr_gid))
> +    return false;
> +
> +  return pstat->st_mode & S_IWGRP;
> +}
> +#else
> +static bool
> +is_tty_writable (const struct stat *pstat)
> +{
> +  return pstat->st_mode & S_IWGRP;
> +}
> +#endif
> +
>  /* Send properly parsed USER_PROCESS info to print_line.  The most
>     recent boot time is BOOTTIME. */
>  static void
> @@ -346,7 +368,7 @@ print_user (const STRUCT_UTMP *utmp_ent, time_t boottime)
>
>    if (stat (line, &stats) == 0)
>      {
> -      mesg = (stats.st_mode & S_IWGRP) ? '+' : '-';
> +      mesg = is_tty_writable (&stats) ? '+' : '-';
>        last_change = stats.st_atime;
>      }
>    else