Re: [PATCH v2 1/2] Allow deriving the current user's home directory via the HOME variable

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

Hi Johannes,

a few comments...

On Dec 17 19:05, Johannes Schindelin wrote:
> [...]
> diff --git a/winsup/cygwin/uinfo.cc b/winsup/cygwin/uinfo.cc
> index c9b3e09..a5d6270 100644
> --- a/winsup/cygwin/uinfo.cc
> +++ b/winsup/cygwin/uinfo.cc
> [...]
> +static size_t
> +fetch_env(LPCWSTR key, char *buf, size_t size)
           ^^^
           space

> +{
> +  WCHAR wbuf[32767];

Ok, there are a couple of problems here.  First, since this buffer
is a filename buffer, use NT_MAX_PATH from winsup.h as buffer size.

But then again, please avoid allocating 64K buffers on the stack.
That's what tmp_pathbuf:w_get () is for.

> +  DWORD max = sizeof wbuf / sizeof *wbuf;
> +  DWORD len = GetEnvironmentVariableW (key, wbuf, max);

This call to GetEnvironmentVariableW looks gratuitous to me.  Why don't
you simply call getenv?  It did the entire job already, it avoids the
requirement for a local buffer, and in case of $HOME it even did the
Win32->POSIX path conversion.  If there's a really good reason for using
GetEnvironmentVariableW it begs at least for a longish comment.

> +
> +  if (!len || len >= max)
> +    return 0;
> +
> +  len = sys_wcstombs (buf, size, wbuf, len);
> +  return len && len < size ? len : 0;
> +}
> +
> +static char *
> +fetch_home_env (void)
> +{
> +  char home[32767];
> +  size_t max = sizeof home / sizeof *home, len;
> +
> +  if (fetch_env (L"HOME", home, max)
> +      || ((len = fetch_env (L"HOMEDRIVE", home, max))
> +        && fetch_env (L"HOMEPATH", home + len, max - len))
> +      || fetch_env (L"USERPROFILE", home, max))
> +    {
> +      tmp_pathbuf tp;
> +      cygwin_conv_path (CCP_WIN_A_TO_POSIX | CCP_ABSOLUTE,
> +	  home, tp.c_get(), NT_MAX_PATH);
                       ^^^
                       space
> +      return strdup(tp.c_get());
                     ^^^      ^^^
                     space......s

Whoa, tp.c_get() twice to access the same space?  That's a dirty trick
which may puzzle later readers of the code and heavily depends on
knowing the internals of tmp_pathbuf.  Please use a variable and only
assign tp.c_get () once.

OTOH, the above's a case for a cygwin_create_path call, rather than
cygwin_conv_path+strdup.  Also, if there's *really* a good reason to use
GetEnvironmentVariableW, you should collapse sys_wcstombs+cygwin_conv_path+
strdup into a single cygwin_create_path (CCP_WIN_W_TO_POSIX, ...).

> [...]
> @@ -1079,6 +1123,7 @@ cygheap_pwdgrp::get_shell (cyg_ldap *pldap, cygpsid &sid, PCWSTR dom,
>  	case NSS_SCHEME_FALLBACK:
>  	  return NULL;
>  	case NSS_SCHEME_WINDOWS:
> +	case NSS_SCHEME_ENV:
>  	  break;
>  	case NSS_SCHEME_CYGWIN:
>  	  if (pldap->fetch_ad_account (sid, false, dnsdomain))

You know that I don't exactly like the "env" idea, but if we implement
it anyway, wouldn't it make sense to add some kind of $SHELL handling as
well, for symmetry?

> [...]
> @@ -1487,6 +1497,16 @@ of each schema when used with <literal>db_home:</literal>
>  	      for a detailed description.</listitem>
>    </varlistentry>
>    <varlistentry>
> +    <term><literal>env</literal></term>
> +    <listitem>Derives the home directory of the current user from the
> +	      environment variable <literal>HOME</literal> (falling back to
> +	      <literal>HOMEDRIVE\HOMEPATH</literal> and
> +	      <literal>USERPROFILE</literal>, in that order).  This is faster
> +	      than the <term><literal>windows</literal></term> schema at the
> +	      expense of determining only the current user's home directory
> +	      correctly.</listitem>

In both case of the documentation it might make sense to add a few words
along the lines of "This schema is skipped for any other account",
wouldn't it?


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: pgp2zp4T89Y2O.pgp
Description: PGP signature