[PATCH 0/3] Add support for /proc/<pid>/environ

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 0/3] Add support for /proc/<pid>/environ

Erik Bray
From: "Erik M. Bray" <[hidden email]>

Per this discussion started in this thread: https://cygwin.com/ml/cygwin/2016-11/msg00205.html

I finally got around to finishing a patch for this feature. It supports both Cygwin and
native Windows processes, more or less following the example of how /proc/<pid>/cmdline is
implemented.

Erik M. Bray (3):
  Move the core environment parsing of environ_init into a new
    win32env_to_cygenv function.
  Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and
    others.
  Add a /proc/<pid>/environ proc file handler, analogous to
    /proc/<pid>/cmdline.

 winsup/cygwin/environ.cc          | 84 +++++++++++++++++++++---------------
 winsup/cygwin/environ.h           |  2 +
 winsup/cygwin/fhandler_process.cc | 22 ++++++++++
 winsup/cygwin/pinfo.cc            | 89 ++++++++++++++++++++++++++++++++++++++-
 winsup/cygwin/pinfo.h             |  4 +-
 5 files changed, 163 insertions(+), 38 deletions(-)

--
2.8.3

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 1/3] Move the core environment parsing of environ_init into a new win32env_to_cygenv function.

Erik Bray
From: "Erik M. Bray" <[hidden email]>

win32env_to_cygwenv handles converting wchar to char and some other
minor taks.  Optionally it handles converting any paths in variables to
posix paths.

This will be useful for implementing /proc/<pid>/environ
---
 winsup/cygwin/environ.cc | 84 ++++++++++++++++++++++++++++--------------------
 winsup/cygwin/environ.h  |  2 ++
 2 files changed, 51 insertions(+), 35 deletions(-)

diff --git a/winsup/cygwin/environ.cc b/winsup/cygwin/environ.cc
index 51107c5..c4195ed 100644
--- a/winsup/cygwin/environ.cc
+++ b/winsup/cygwin/environ.cc
@@ -758,18 +758,12 @@ ucenv (char *p, const char *eq)
 void
 environ_init (char **envp, int envc)
 {
-  PWCHAR rawenv, w;
-  int i;
+  PWCHAR rawenv;
   char *p;
-  char *newp;
-  int sawTERM = 0;
   bool envp_passed_in;
-  static char NO_COPY cygterm[] = "TERM=cygwin";
-  tmp_pathbuf tp;
 
   __try
     {
-      char *tmpbuf = tp.t_get ();
       if (!envp)
  envp_passed_in = 0;
       else
@@ -794,9 +788,6 @@ environ_init (char **envp, int envc)
   goto out;
  }
 
-      /* Allocate space for environment + trailing NULL + CYGWIN env. */
-      lastenviron = envp = (char **) malloc ((4 + (envc = 100)) * sizeof (char *));
-
       rawenv = GetEnvironmentStringsW ();
       if (!rawenv)
  {
@@ -805,32 +796,8 @@ environ_init (char **envp, int envc)
  }
       debug_printf ("GetEnvironmentStrings returned %p", rawenv);
 
-      /* Current directory information is recorded as variables of the
- form "=X:=X:\foo\bar; these must be changed into something legal
- (we could just ignore them but maybe an application will
- eventually want to use them).  */
-      for (i = 0, w = rawenv; *w != L'\0'; w = wcschr (w, L'\0') + 1, i++)
- {
-  sys_wcstombs_alloc_no_path (&newp, HEAP_NOTHEAP, w);
-  if (i >= envc)
-    envp = (char **) realloc (envp, (4 + (envc += 100)) * sizeof (char *));
-  envp[i] = newp;
-  if (*newp == '=')
-    *newp = '!';
-  char *eq = strchrnul (newp, '=');
-  ucenv (newp, eq); /* uppercase env vars which need it */
-  if (*newp == 'T' && strncmp (newp, "TERM=", 5) == 0)
-    sawTERM = 1;
-  else if (*newp == 'C' && strncmp (newp, "CYGWIN=", 7) == 0)
-    parse_options (newp + 7);
-  if (*eq)
-    posify_maybe (envp + i, *++eq ? eq : --eq, tmpbuf);
-  debug_printf ("%p: %s", envp[i], envp[i]);
- }
+  lastenviron = envp = win32env_to_cygenv(rawenv, true);
 
-      if (!sawTERM)
- envp[i++] = strdup (cygterm);
-      envp[i] = NULL;
       FreeEnvironmentStringsW (rawenv);
 
     out:
@@ -852,6 +819,53 @@ environ_init (char **envp, int envc)
   __endtry
 }
 
+
+char **
+win32env_to_cygenv(PWCHAR rawenv, bool posify)
+{
+  tmp_pathbuf tp;
+  char **envp;
+  int envc;
+  char *newp;
+  int i;
+  int sawTERM = 0;
+  static char NO_COPY cygterm[] = "TERM=cygwin";
+  char *tmpbuf = tp.t_get ();
+  PWCHAR w;
+
+  /* Allocate space for environment + trailing NULL + CYGWIN env. */
+  envp = (char **) malloc ((4 + (envc = 100)) * sizeof (char *));
+
+  /* Current directory information is recorded as variables of the
+     form "=X:=X:\foo\bar; these must be changed into something legal
+     (we could just ignore them but maybe an application will
+     eventually want to use them).  */
+  for (i = 0, w = rawenv; *w != L'\0'; w = wcschr (w, L'\0') + 1, i++)
+    {
+      sys_wcstombs_alloc_no_path (&newp, HEAP_NOTHEAP, w);
+      if (i >= envc)
+        envp = (char **) realloc (envp, (4 + (envc += 100)) * sizeof (char *));
+      envp[i] = newp;
+      if (*newp == '=')
+        *newp = '!';
+      char *eq = strchrnul (newp, '=');
+      ucenv (newp, eq);    /* uppercase env vars which need it */
+      if (*newp == 'T' && strncmp (newp, "TERM=", 5) == 0)
+        sawTERM = 1;
+      else if (*newp == 'C' && strncmp (newp, "CYGWIN=", 7) == 0)
+        parse_options (newp + 7);
+      if (*eq && posify)
+        posify_maybe (envp + i, *++eq ? eq : --eq, tmpbuf);
+      debug_printf ("%p: %s", envp[i], envp[i]);
+    }
+
+  if (!sawTERM)
+    envp[i++] = strdup (cygterm);
+
+  envp[i] = NULL;
+  return envp;
+}
+
 /* Function called by qsort to sort environment strings.  */
 static int
 env_sort (const void *a, const void *b)
diff --git a/winsup/cygwin/environ.h b/winsup/cygwin/environ.h
index 46beb2d..7bd87da 100644
--- a/winsup/cygwin/environ.h
+++ b/winsup/cygwin/environ.h
@@ -45,4 +45,6 @@ extern "C" char __stdcall **cur_environ ();
 char ** __reg3 build_env (const char * const *envp, PWCHAR &envblock,
   int &envc, bool need_envblock, HANDLE new_token);
 
+char __stdcall ** win32env_to_cygenv (PWCHAR rawenv, bool posify);
+
 #define ENV_CVT -1
--
2.8.3

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 2/3] Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and others.

Erik Bray
In reply to this post by Erik Bray
From: "Erik M. Bray" <[hidden email]>

Returns the process's environment concatenated into a single block of
null-terminated strings, along with the length of the environment block.

Adds an associated PICOM_ENVIRON commune_process handler.
---
 winsup/cygwin/pinfo.cc | 89 ++++++++++++++++++++++++++++++++++++++++++++++++--
 winsup/cygwin/pinfo.h  |  4 ++-
 2 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index 1ce6809..a3e376c 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -653,8 +653,29 @@ commune_process (void *arg)
  else if (!WritePipeOverlapped (tothem, path, n, &nr, 1000L))
   sigproc_printf ("WritePipeOverlapped fd failed, %E");
  break;
-      }
-    }
+  }
+ case PICOM_ENVIRON:
+  {
+ sigproc_printf ("processing PICOM_ENVIRON");
+ unsigned n = 0;
+    char **env = cur_environ ();
+    for (char **e = env; *e; e++)
+        n += strlen (*e) + 1;
+ if (!WritePipeOverlapped (tothem, &n, sizeof n, &nr, 1000L))
+  {
+    sigproc_printf ("WritePipeOverlapped sizeof argv failed, %E");
+  }
+ else
+  for (char **e = env; *e; e++)
+    if (!WritePipeOverlapped (tothem, *e, strlen (*e) + 1, &nr, 1000L))
+      {
+        sigproc_printf ("WritePipeOverlapped arg %d failed, %E",
+                        e - env);
+ break;
+      }
+ break;
+  }
+ }
   if (process_sync)
     {
       DWORD res = WaitForSingleObject (process_sync, 5000);
@@ -730,6 +751,7 @@ _pinfo::commune_request (__uint32_t code, ...)
     {
     case PICOM_CMDLINE:
     case PICOM_CWD:
+    case PICOM_ENVIRON:
     case PICOM_ROOT:
     case PICOM_FDS:
     case PICOM_FD:
@@ -993,6 +1015,69 @@ _pinfo::cmdline (size_t& n)
   return s;
 }
 
+
+char *
+_pinfo::environ (size_t& n)
+{
+  char **env = NULL;
+  if (!this || !pid)
+    return NULL;
+  if (ISSTATE (this, PID_NOTCYGWIN))
+    {
+      RTL_USER_PROCESS_PARAMETERS rupp;
+      HANDLE proc = open_commune_proc_parms (dwProcessId, &rupp);
+
+      n = 0;
+      if (!proc)
+        return NULL;
+
+  MEMORY_BASIC_INFORMATION mbi;
+      SIZE_T envsize;
+      PWCHAR envblock;
+  if (!VirtualQueryEx (proc, rupp.Environment, &mbi, sizeof(mbi)))
+        {
+          NtClose (proc);
+          return NULL;
+        }
+
+      SIZE_T read;
+      envsize = mbi.RegionSize - ((char*) rupp.Environment - (char*) mbi.BaseAddress);
+      envblock = (PWCHAR) cmalloc_abort (HEAP_COMMUNE, envsize);
+
+      if (ReadProcessMemory (proc, rupp.Environment, envblock, envsize, &read))
+        {
+          env = win32env_to_cygenv (envblock, false);
+        }
+
+      NtClose (proc);
+    }
+  else if (pid != myself->pid)
+    {
+      commune_result cr = commune_request (PICOM_ENVIRON);
+      n = cr.n;
+      return cr.s;
+    }
+  else
+    {
+      env = cur_environ ();
+    }
+
+  if (env == NULL)
+      return NULL;
+
+  for (char **e = env; *e; e++)
+    n += strlen (*e) + 1;
+
+  char *p, *s;
+  p = s = (char *) cmalloc_abort (HEAP_COMMUNE, n);
+  for (char **e = env; *e; e++)
+    {
+      strcpy (p, *e);
+      p = strchr (p, '\0') + 1;
+    }
+  return s;
+}
+
 /* This is the workhorse which waits for the write end of the pipe
    created during new process creation.  If the pipe is closed or a zero
    is received on the pipe, it is assumed that the cygwin pid has exited.
diff --git a/winsup/cygwin/pinfo.h b/winsup/cygwin/pinfo.h
index fd76c8d..7ad1294 100644
--- a/winsup/cygwin/pinfo.h
+++ b/winsup/cygwin/pinfo.h
@@ -26,7 +26,8 @@ enum picom
   PICOM_ROOT = 3,
   PICOM_FDS = 4,
   PICOM_FD = 5,
-  PICOM_PIPE_FHANDLER = 6
+  PICOM_PIPE_FHANDLER = 6,
+  PICOM_ENVIRON = 7
 };
 
 #define EXITCODE_SET 0x8000000
@@ -106,6 +107,7 @@ public:
   char *root (size_t &);
   char *cwd (size_t &);
   char *cmdline (size_t &);
+  char *environ (size_t &);
   char *win_heap_info (size_t &);
   bool set_ctty (class fhandler_termios *, int);
   bool alert_parent (char);
--
2.8.3

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH 3/3] Add a /proc/<pid>/environ proc file handler, analogous to /proc/<pid>/cmdline.

Erik Bray
In reply to this post by Erik Bray
From: "Erik M. Bray" <[hidden email]>

---
 winsup/cygwin/fhandler_process.cc | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/winsup/cygwin/fhandler_process.cc b/winsup/cygwin/fhandler_process.cc
index 5f530a2..bbb44fa 100644
--- a/winsup/cygwin/fhandler_process.cc
+++ b/winsup/cygwin/fhandler_process.cc
@@ -49,6 +49,7 @@ static off_t format_process_ctty (void *, char *&);
 static off_t format_process_fd (void *, char *&);
 static off_t format_process_mounts (void *, char *&);
 static off_t format_process_mountinfo (void *, char *&);
+static off_t format_process_environ (void *, char *&);
 
 static const virt_tab_t process_tab[] =
 {
@@ -57,6 +58,7 @@ static const virt_tab_t process_tab[] =
   { _VN ("cmdline"),    FH_PROCESS,   virt_file,      format_process_cmdline },
   { _VN ("ctty"),       FH_PROCESS,   virt_file,      format_process_ctty },
   { _VN ("cwd"),        FH_PROCESS,   virt_symlink,   format_process_cwd },
+  { _VN ("environ"),    FH_PROCESS,   virt_file,      format_process_environ },
   { _VN ("exe"),        FH_PROCESS,   virt_symlink,   format_process_exename },
   { _VN ("exename"),    FH_PROCESS,   virt_file,      format_process_exename },
   { _VN ("fd"),         FH_PROCESSFD, virt_directory, format_process_fd },
@@ -570,6 +572,26 @@ format_process_winexename (void *data, char *&destbuf)
   return len;
 }
 
+static off_t
+format_process_environ (void *data, char *&destbuf)
+{
+  _pinfo *p = (_pinfo *) data;
+  size_t fs;
+
+  if (destbuf)
+    {
+      cfree (destbuf);
+      destbuf = NULL;
+    }
+  destbuf = p->environ (fs);
+  if (!destbuf || !*destbuf)
+    {
+      destbuf = cstrdup ("<defunct>");
+      fs = strlen (destbuf) + 1;
+    }
+  return fs;
+}
+
 struct heap_info
 {
   struct heap
--
2.8.3

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 0/3] Add support for /proc/<pid>/environ

Corinna Vinschen-2
In reply to this post by Erik Bray
Hi Erik,

On Jan  5 18:39, [hidden email] wrote:

> From: "Erik M. Bray" <[hidden email]>
>
> Per this discussion started in this thread: https://cygwin.com/ml/cygwin/2016-11/msg00205.html
>
> I finally got around to finishing a patch for this feature. It supports both Cygwin and
> native Windows processes, more or less following the example of how /proc/<pid>/cmdline is
> implemented.
>
> Erik M. Bray (3):
>   Move the core environment parsing of environ_init into a new
>     win32env_to_cygenv function.
>   Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and
>     others.
>   Add a /proc/<pid>/environ proc file handler, analogous to
>     /proc/<pid>/cmdline.
>
>  winsup/cygwin/environ.cc          | 84 +++++++++++++++++++++---------------
>  winsup/cygwin/environ.h           |  2 +
>  winsup/cygwin/fhandler_process.cc | 22 ++++++++++
>  winsup/cygwin/pinfo.cc            | 89 ++++++++++++++++++++++++++++++++++++++-
>  winsup/cygwin/pinfo.h             |  4 +-
>  5 files changed, 163 insertions(+), 38 deletions(-)
Patch looks good basically, but I have a few nits:

- We need your 2-clause BSD license text per the "Before you get started"
  section of https://cygwin.com/contrib.html.  For the text see
  https://cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/CONTRIBUTORS

- While this appears to work nicely on other processes, it seems to be
  broken on the process itself.  Did you try `cat /proc/self/environ'?
  I'm getting a "Bad address" error when trying that.

- A few formatting issues, see my next replies.

Other than that, thanks for this nice addition!


Corinna

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 1/3] Move the core environment parsing of environ_init into a new win32env_to_cygenv function.

Corinna Vinschen-2
In reply to this post by Erik Bray
On Jan  5 18:39, Erik M. Bray wrote:

> @@ -805,32 +796,8 @@ environ_init (char **envp, int envc)
>   }
>        debug_printf ("GetEnvironmentStrings returned %p", rawenv);
>  
> -      /* Current directory information is recorded as variables of the
> - form "=X:=X:\foo\bar; these must be changed into something legal
> - (we could just ignore them but maybe an application will
> - eventually want to use them).  */
> -      for (i = 0, w = rawenv; *w != L'\0'; w = wcschr (w, L'\0') + 1, i++)
> - {
> -  sys_wcstombs_alloc_no_path (&newp, HEAP_NOTHEAP, w);
> -  if (i >= envc)
> -    envp = (char **) realloc (envp, (4 + (envc += 100)) * sizeof (char *));
> -  envp[i] = newp;
> -  if (*newp == '=')
> -    *newp = '!';
> -  char *eq = strchrnul (newp, '=');
> -  ucenv (newp, eq); /* uppercase env vars which need it */
> -  if (*newp == 'T' && strncmp (newp, "TERM=", 5) == 0)
> -    sawTERM = 1;
> -  else if (*newp == 'C' && strncmp (newp, "CYGWIN=", 7) == 0)
> -    parse_options (newp + 7);
> -  if (*eq)
> -    posify_maybe (envp + i, *++eq ? eq : --eq, tmpbuf);
> -  debug_printf ("%p: %s", envp[i], envp[i]);
> - }
> +  lastenviron = envp = win32env_to_cygenv(rawenv, true);
                                                ^^^
                                                space missing

>  
> -      if (!sawTERM)
> - envp[i++] = strdup (cygterm);
> -      envp[i] = NULL;
>        FreeEnvironmentStringsW (rawenv);
>  
>      out:
> @@ -852,6 +819,53 @@ environ_init (char **envp, int envc)
>    __endtry
>  }
>  
> +
> +char **
      ^^^
      Header claims __stdcall, missing here.  But in fact it
      might be prudent to make this a __reg2 function instead.

> +win32env_to_cygenv(PWCHAR rawenv, bool posify)
                    ^^^
                    space missing

> +{

>  /* Function called by qsort to sort environment strings.  */
>  static int
>  env_sort (const void *a, const void *b)
> diff --git a/winsup/cygwin/environ.h b/winsup/cygwin/environ.h
> index 46beb2d..7bd87da 100644
> --- a/winsup/cygwin/environ.h
> +++ b/winsup/cygwin/environ.h
> @@ -45,4 +45,6 @@ extern "C" char __stdcall **cur_environ ();
>  char ** __reg3 build_env (const char * const *envp, PWCHAR &envblock,
>    int &envc, bool need_envblock, HANDLE new_token);
>  
> +char __stdcall ** win32env_to_cygenv (PWCHAR rawenv, bool posify);
        ^^^^^      ^^^
        __reg2?    no space here


Thanks,
Corinna

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 2/3] Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and others.

Corinna Vinschen-2
In reply to this post by Erik Bray
On Jan  5 18:39, Erik M. Bray wrote:

> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
> index 1ce6809..a3e376c 100644
> --- a/winsup/cygwin/pinfo.cc
> +++ b/winsup/cygwin/pinfo.cc
> @@ -653,8 +653,29 @@ commune_process (void *arg)
>   else if (!WritePipeOverlapped (tothem, path, n, &nr, 1000L))
>    sigproc_printf ("WritePipeOverlapped fd failed, %E");
>   break;
> -      }
> -    }
> +  }
> + case PICOM_ENVIRON:
> +  {
> + sigproc_printf ("processing PICOM_ENVIRON");
> + unsigned n = 0;
> +    char **env = cur_environ ();
> +    for (char **e = env; *e; e++)
> +        n += strlen (*e) + 1;
> + if (!WritePipeOverlapped (tothem, &n, sizeof n, &nr, 1000L))
> +  {
> +    sigproc_printf ("WritePipeOverlapped sizeof argv failed, %E");
> +  }
          No curlies here, please, just as in sibling cases.

> + else
> +  for (char **e = env; *e; e++)
> +    if (!WritePipeOverlapped (tothem, *e, strlen (*e) + 1, &nr, 1000L))
> +      {
> +        sigproc_printf ("WritePipeOverlapped arg %d failed, %E",
> +                        e - env);
> + break;
> +      }
> + break;
> +  }
> + }
Please have another look into the PICOM_ENVIRON case.  Indentation is
completely broken in this code snippet, as if it has been moved around
a bit and then left at the wrong spot.

>  }
>  
> +
> +char *
> +_pinfo::environ (size_t& n)
> +{
> +  char **env = NULL;
> +  if (!this || !pid)
> +    return NULL;
> +  if (ISSTATE (this, PID_NOTCYGWIN))
> +    {
> +      RTL_USER_PROCESS_PARAMETERS rupp;
> +      HANDLE proc = open_commune_proc_parms (dwProcessId, &rupp);
> +
> +      n = 0;
> +      if (!proc)
> +        return NULL;
> +
> +  MEMORY_BASIC_INFORMATION mbi;
Whoops, broken indentation again.

> +      SIZE_T envsize;
> +      PWCHAR envblock;
> +  if (!VirtualQueryEx (proc, rupp.Environment, &mbi, sizeof(mbi)))

And here

> +        {
> +          NtClose (proc);
> +          return NULL;
> +        }
> +
> +      SIZE_T read;
> +      envsize = mbi.RegionSize - ((char*) rupp.Environment - (char*) mbi.BaseAddress);

Stick to max 80 chars per line, please, i.e.

  +      envsize = mbi.RegionSize
                   - ((char*) rupp.Environment - (char*) mbi.BaseAddress);

It might also be prudent to use another cast here, like, say, ptrdiff_t,
since this is for pointer arithmetic only anyway.

> +      envblock = (PWCHAR) cmalloc_abort (HEAP_COMMUNE, envsize);
> +
> +      if (ReadProcessMemory (proc, rupp.Environment, envblock, envsize, &read))
> +        {
> +          env = win32env_to_cygenv (envblock, false);
> +        }

Your function, your choice, but I'd get rid of the curly braces for
oneliners here.

> +
> +      NtClose (proc);
> +    }
> +  else if (pid != myself->pid)
> +    {
> +      commune_result cr = commune_request (PICOM_ENVIRON);
> +      n = cr.n;
> +      return cr.s;
> +    }
> +  else
> +    {
> +      env = cur_environ ();
> +    }
Same here.

> +
> +  if (env == NULL)
> +      return NULL;

Just as here.


Corinna

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 0/3] Add support for /proc/<pid>/environ

Erik Bray
In reply to this post by Corinna Vinschen-2
On Mon, Jan 9, 2017 at 3:43 PM, Corinna Vinschen
<[hidden email]> wrote:

> Hi Erik,
>
> On Jan  5 18:39, [hidden email] wrote:
>> From: "Erik M. Bray" <[hidden email]>
>>
>> Per this discussion started in this thread: https://cygwin.com/ml/cygwin/2016-11/msg00205.html
>>
>> I finally got around to finishing a patch for this feature. It supports both Cygwin and
>> native Windows processes, more or less following the example of how /proc/<pid>/cmdline is
>> implemented.
>>
>> Erik M. Bray (3):
>>   Move the core environment parsing of environ_init into a new
>>     win32env_to_cygenv function.
>>   Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and
>>     others.
>>   Add a /proc/<pid>/environ proc file handler, analogous to
>>     /proc/<pid>/cmdline.
>>
>>  winsup/cygwin/environ.cc          | 84 +++++++++++++++++++++---------------
>>  winsup/cygwin/environ.h           |  2 +
>>  winsup/cygwin/fhandler_process.cc | 22 ++++++++++
>>  winsup/cygwin/pinfo.cc            | 89 ++++++++++++++++++++++++++++++++++++++-
>>  winsup/cygwin/pinfo.h             |  4 +-
>>  5 files changed, 163 insertions(+), 38 deletions(-)
>
> Patch looks good basically, but I have a few nits:
>
> - We need your 2-clause BSD license text per the "Before you get started"
>   section of https://cygwin.com/contrib.html.  For the text see
>   https://cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/CONTRIBUTORS
>
> - While this appears to work nicely on other processes, it seems to be
>   broken on the process itself.  Did you try `cat /proc/self/environ'?
>   I'm getting a "Bad address" error when trying that.
>
> - A few formatting issues, see my next replies.
>
> Other than that, thanks for this nice addition!

Incidentally, I don't think I did test `/proc/self/environ`.  I'll
certainly fix whatever's wrong with that.

When I fix that and the issues you pointed out on the other patches,
should I just submit a new patch set?  When I do so I can also include
the BSD license statement.

Thanks,
Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 0/3] Add support for /proc/<pid>/environ

Corinna Vinschen-2
On Jan 10 11:48, Erik Bray wrote:

> On Mon, Jan 9, 2017 at 3:43 PM, Corinna Vinschen
> <[hidden email]> wrote:
> > Hi Erik,
> >
> > On Jan  5 18:39, [hidden email] wrote:
> >> From: "Erik M. Bray" <[hidden email]>
> >>
> >> Per this discussion started in this thread: https://cygwin.com/ml/cygwin/2016-11/msg00205.html
> >>
> >> I finally got around to finishing a patch for this feature. It supports both Cygwin and
> >> native Windows processes, more or less following the example of how /proc/<pid>/cmdline is
> >> implemented.
> >>
> >> Erik M. Bray (3):
> >>   Move the core environment parsing of environ_init into a new
> >>     win32env_to_cygenv function.
> >>   Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and
> >>     others.
> >>   Add a /proc/<pid>/environ proc file handler, analogous to
> >>     /proc/<pid>/cmdline.
> >>
> >>  winsup/cygwin/environ.cc          | 84 +++++++++++++++++++++---------------
> >>  winsup/cygwin/environ.h           |  2 +
> >>  winsup/cygwin/fhandler_process.cc | 22 ++++++++++
> >>  winsup/cygwin/pinfo.cc            | 89 ++++++++++++++++++++++++++++++++++++++-
> >>  winsup/cygwin/pinfo.h             |  4 +-
> >>  5 files changed, 163 insertions(+), 38 deletions(-)
> >
> > Patch looks good basically, but I have a few nits:
> >
> > - We need your 2-clause BSD license text per the "Before you get started"
> >   section of https://cygwin.com/contrib.html.  For the text see
> >   https://cygwin.com/git/?p=newlib-cygwin.git;a=blob;f=winsup/CONTRIBUTORS
> >
> > - While this appears to work nicely on other processes, it seems to be
> >   broken on the process itself.  Did you try `cat /proc/self/environ'?
> >   I'm getting a "Bad address" error when trying that.
> >
> > - A few formatting issues, see my next replies.
> >
> > Other than that, thanks for this nice addition!
>
> Incidentally, I don't think I did test `/proc/self/environ`.  I'll
> certainly fix whatever's wrong with that.
>
> When I fix that and the issues you pointed out on the other patches,
> should I just submit a new patch set?  When I do so I can also include
> the BSD license statement.
Sounds good!


Thanks,
Corinna

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

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 2/3] Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and others.

Erik Bray
In reply to this post by Corinna Vinschen-2
On Mon, Jan 9, 2017 at 3:58 PM, Corinna Vinschen
<[hidden email]> wrote:

> On Jan  5 18:39, Erik M. Bray wrote:
>> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
>> index 1ce6809..a3e376c 100644
>> --- a/winsup/cygwin/pinfo.cc
>> +++ b/winsup/cygwin/pinfo.cc
>> @@ -653,8 +653,29 @@ commune_process (void *arg)
>>       else if (!WritePipeOverlapped (tothem, path, n, &nr, 1000L))
>>         sigproc_printf ("WritePipeOverlapped fd failed, %E");
>>       break;
>> -      }
>> -    }
>> +       }
>> +     case PICOM_ENVIRON:
>> +       {
>> +     sigproc_printf ("processing PICOM_ENVIRON");
>> +     unsigned n = 0;
>> +    char **env = cur_environ ();
>> +    for (char **e = env; *e; e++)
>> +        n += strlen (*e) + 1;
>> +     if (!WritePipeOverlapped (tothem, &n, sizeof n, &nr, 1000L))
>> +       {
>> +         sigproc_printf ("WritePipeOverlapped sizeof argv failed, %E");
>> +       }
>
>           No curlies here, please, just as in sibling cases.
>
>> +     else
>> +       for (char **e = env; *e; e++)
>> +         if (!WritePipeOverlapped (tothem, *e, strlen (*e) + 1, &nr, 1000L))
>> +           {
>> +             sigproc_printf ("WritePipeOverlapped arg %d failed, %E",
>> +                             e - env);
>> +             break;
>> +           }
>> +     break;
>> +       }
>> +     }
>
> Please have another look into the PICOM_ENVIRON case.  Indentation is
> completely broken in this code snippet, as if it has been moved around
> a bit and then left at the wrong spot.

One note on indentation: I tried to be consistent but it's hard
because in that file and others there's a lot of mixing of tabs and
spaces.  I'm happy to get everything cleaned up, I'm just not sure
what the "intended" convention is wrt tabs vs. spaces (I know you're
using the GNU coding standards otherwise).

Would you welcome a separate patch with general whitespace cleanup?

I acknowledge and can fix every other point.

Thanks,
Erik
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH 2/3] Add a _pinfo.environ() method analogous to _pinfo.cmdline(), and others.

Corinna Vinschen-2
On Jan 10 11:56, Erik Bray wrote:

> On Mon, Jan 9, 2017 at 3:58 PM, Corinna Vinschen
> <[hidden email]> wrote:
> > On Jan  5 18:39, Erik M. Bray wrote:
> >> diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
> >> index 1ce6809..a3e376c 100644
> >> --- a/winsup/cygwin/pinfo.cc
> >> +++ b/winsup/cygwin/pinfo.cc
> >> @@ -653,8 +653,29 @@ commune_process (void *arg)
> >>       else if (!WritePipeOverlapped (tothem, path, n, &nr, 1000L))
> >>         sigproc_printf ("WritePipeOverlapped fd failed, %E");
> >>       break;
> >> -      }
> >> -    }
> >> +       }
> >> +     case PICOM_ENVIRON:
> >> +       {
> >> +     sigproc_printf ("processing PICOM_ENVIRON");
> >> +     unsigned n = 0;
> >> +    char **env = cur_environ ();
> >> +    for (char **e = env; *e; e++)
> >> +        n += strlen (*e) + 1;
> >> +     if (!WritePipeOverlapped (tothem, &n, sizeof n, &nr, 1000L))
> >> +       {
> >> +         sigproc_printf ("WritePipeOverlapped sizeof argv failed, %E");
> >> +       }
> >
> >           No curlies here, please, just as in sibling cases.
> >
> >> +     else
> >> +       for (char **e = env; *e; e++)
> >> +         if (!WritePipeOverlapped (tothem, *e, strlen (*e) + 1, &nr, 1000L))
> >> +           {
> >> +             sigproc_printf ("WritePipeOverlapped arg %d failed, %E",
> >> +                             e - env);
> >> +             break;
> >> +           }
> >> +     break;
> >> +       }
> >> +     }
> >
> > Please have another look into the PICOM_ENVIRON case.  Indentation is
> > completely broken in this code snippet, as if it has been moved around
> > a bit and then left at the wrong spot.
>
> One note on indentation: I tried to be consistent but it's hard
> because in that file and others there's a lot of mixing of tabs and
> spaces.  I'm happy to get everything cleaned up, I'm just not sure
> what the "intended" convention is wrt tabs vs. spaces (I know you're
> using the GNU coding standards otherwise).
It's not that tricky.  In vim terms, set ts=8 sw=2, and use tabs
if the indentation is >= 8.  If you do a >> << sequence in vim,
you get it right even if it was wrong before.

> Would you welcome a separate patch with general whitespace cleanup?

It's new code so that doesn't make much sense.  Otherwise cleaning
up existing stuff as separate patch is fine.


Corinna

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

signature.asc (836 bytes) Download Attachment
Loading...