265219 - No support for redirecting stdin/stdout of a child process

Status: RESOLVED FIXED

Description

[Davide Bresolin]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; it-IT; rv:1.7) Gecko/20040629 Firefox/0.9.1
Build Identifier: Thunderbird 0.8

This bugs involves porting Enigmail (http://enigmail.mozdev.org) to OS/2.
In order to use GNUpg to crypy/dectpt messages, enigmail must:
 1. start gnupg, and wait for termination
 2. communicate with gnupg by redirecting stdin/stdout of the child process.
These features are currently not implemented in the OS/2 port of NSPR

Reproducible: Always
Steps to Reproduce:
1. Install my enigmail port for OS/2 (downloadable at
http://www.dimi.uniud.it/bresolin/warpzilla);
2. try to generate a new key for an account, or to crypt or sign a message.


Actual Results:  
Enigmail starts gnupg, but it doesn't wait for its termination nor it will be
able to coomuncate with gnupg. It returns an error, reporting this.

Expected Results:  
Start gnupg, wait for the result and communicate with gnupg via stdin/stdout
redirection

Comment 1

[Davide Bresolin]

Created attachment 162695 [details] [diff] [review]
patch to os2misc.c to add support for stdin/stdout redirection

I've added a patch file for nsprpub/src/pr/src/md/os2/os2misc.c that fixes the
problem.
It changes the code of _PR_OS2CreateProcess and, when stdin/stdout redirecting
is needed, it uses DosExecPgm instead of DosStartSession, allowing the program
to redirect stdin/stdout. 

Furthermore, in this case _PR_WaitOS2Process works correctly.

Comment 2

[Davide Bresolin]

Created attachment 162701 [details] [diff] [review]
Better os2misc.diff, extracted by CVS

Comment 3

[Peter Weilbacher]

Great! I will take a closer look at the patch and try it out, hopefully tonight.

Comment 4

[Peter Weilbacher]

As I didn't have a Thunderbird tree around I tried this patch on the trunk. The
lines are different but it compiles. I could not try it out yet because your TB
enigmail XPIs do not work with the current trunk. Did you need to patch
enigmail, too? If so, could open a bug on that on mozdev and attach the patch?
If not I will just compile it from their current sources.

Some comments on the NSPR patch (I am not familar with that code, so this is not
a real review):

- Could you not just use rc instead of the new variable retVal?
        retVal = DosExecPgm(szFailed,
                            CCHMAXPATH,
                            EXEC_ASYNCRESULT,
                            pszCmdLine,
                            envBlock,
                            &procInfo,
                            pszEXEName);
  Are you sure that you get a sensible result code? The API reference 
  says to use DosWaitChild for that in the case of EXEC_ASYNCRESULT.
- How about using
            PR_SetError(PR_UNKNOWN_ERROR, 0);
  for this
        if (retVal != NO_ERROR) {
            /* XXX what error code? */
            PR_SetError(PR_UNKNOWN_ERROR, retVal);
            goto errorExit;
        }
  as in the else branch?
- With this amount of new lines you should perhaps add yourself to the
  (currently empty) contributors in the header, or is that not done for
  NSPR?

Comment 5

[Davide Bresolin]

(In reply to comment #4)

> enigmail XPIs do not work with the current trunk. Did you need to patch
> enigmail, too? If so, could open a bug on that on mozdev and attach the patch?
> If not I will just compile it from their current sources.
I've patch enigmail too, but it is a minor difference: enigmail places comments
and other command arguments between single quotes ('), while in OS/2 you must
use  double quotes ("). I have also created makefiles by hand, because I cannot
get the included makemake script to work. 
My idea was to create a .cmd script to create makefiles, and then to submit a
bug for the single/double quotes question.

> Some comments on the NSPR patch (I am not familar with that code, so this is not
> a real review):
> 
> - Could you not just use rc instead of the new variable retVal?
Yes, I do. retVal is useless, thanks for the suggestion. I'll upload a new diff.

>   Are you sure that you get a sensible result code? The API reference 
>   says to use DosWaitChild for that in the case of EXEC_ASYNCRESULT.
I think that you get a sensible result code. DosWaitChild reports the result
code of the process you start, while DosExecPgm returns you an error code if the
process cannot be started.

> - How about using
>             PR_SetError(PR_UNKNOWN_ERROR, 0);
>   for this
>         if (retVal != NO_ERROR) {
>             /* XXX what error code? */
>             PR_SetError(PR_UNKNOWN_ERROR, retVal);
>             goto errorExit;
>         }
>   as in the else branch?
I don't know, I've just copied this piece of code. Maybe someone more expert in
NSPR code can answer to this question.

> - With this amount of new lines you should perhaps add yourself to the
>   (currently empty) contributors in the header, or is that not done for
>   NSPR?
How can I add my name to the contributors?

Comment 6

[Davide Bresolin]

Created attachment 162989 [details] [diff] [review]
Better os2misc.diff, removed useless variables

Comment 7

[Peter Weilbacher]

(In reply to comment #5)
> How can I add my name to the contributors?
Just add your name and email address in the line after "Contributors:" in the
header of the C file (didn't find a NSPR file where this is done but an example
is http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileUnicode.h#22).

Comment 8

[Mike Kaply [:mkaply]]

You have used an editor that removes whitespace at the end of the lines.

This is causing many unnecessary things in the patch.

Please redo the diff with the -w flag to avoid the whitespace changes.

Comment 9

[Davide Bresolin]

Created attachment 164533 [details] [diff] [review]
patch to os2misc.c to add support for stdin/stdout redirection

Modified os2misc.diff, removed useless things

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 164533 [details] [diff] [review]
patch to os2misc.c to add support for stdin/stdout redirection

r=mkaply

Comment 11

[Wan-Teh Chang]

Comment on attachment 164533 [details] [diff] [review]
patch to os2misc.c to add support for stdin/stdout redirection

r=wtc.

I checked in this patch on the NSPR trunk (NSPR 4.6)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (post Mozilla 1.8
Alpha 4) after making the following changes.

1. I added Davide Bresolin <[e-mail]> as a
contributor.

2. I wrapped lines longer than 80 characters and fixed
some other indentation problems.

3. I delete pszCmdLine immediately after it is used.

Although I reviewed and checked in this patch, there
is room for improvement.  I found that some of the code,
such as the code to set up various fields in the startData
structure, is not needed for the DosExecPgm case.  But I
didn't spent the time to figure out how to move that code
to the DosStartSession case.

Comment 12

[Wan-Teh Chang]

Created attachment 164649 [details] [diff] [review]
Go to errorExit on DosStartSession failure

I noticed this problem while reviewing Davide's patch.
I think we should also goto errorExit if DosStartSession
failed.  Also, we should pass 'rc' to the PR_SetError
call as the OS error.

Comment 13

[Mike Kaply [:mkaply]]

Comment on attachment 164649 [details] [diff] [review]
Go to errorExit on DosStartSession failure

r=mkaply

Comment 14

[Wan-Teh Chang]

I've checked in the second patch on the NSPR trunk (NSPR 4.6)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (post Mozilla 1.8 Alpha 4).

Marked the bug fixed.