No support for redirecting stdin/stdout of a child process

RESOLVED FIXED in 4.6

Status

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: davide, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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
(Reporter)

Comment 1

14 years ago
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.
(Reporter)

Updated

14 years ago
Attachment #162695 - Attachment filename: os2misc.diff
(Reporter)

Comment 2

14 years ago
Created attachment 162701 [details] [diff] [review]
Better os2misc.diff, extracted by CVS
Attachment #162695 - Attachment is obsolete: true

Comment 3

14 years ago
Great! I will take a closer look at the patch and try it out, hopefully tonight.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

14 years ago
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?
(Reporter)

Comment 5

14 years ago
(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?
(Reporter)

Comment 6

14 years ago
Created attachment 162989 [details] [diff] [review]
Better os2misc.diff, removed useless variables
Attachment #162701 - Attachment is obsolete: true

Comment 7

14 years ago
(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).

Updated

14 years ago
Attachment #162989 - Flags: superreview?(wchang0222)
Attachment #162989 - Flags: review?(mkaply)

Comment 8

14 years ago
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.
(Reporter)

Comment 9

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

Modified os2misc.diff, removed useless things
(Reporter)

Updated

14 years ago
Attachment #162989 - Attachment is obsolete: true

Updated

14 years ago
Attachment #162989 - Flags: superreview?(wchang0222)
Attachment #162989 - Flags: review?(mkaply)

Comment 10

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

r=mkaply
Attachment #164533 - Flags: superreview?(wchang0222)
Attachment #164533 - Flags: review+
(Assignee)

Comment 11

14 years ago
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 <davide@teamos2.it> 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.
Attachment #164533 - Flags: superreview?(wchang0222) → superreview+
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → 4.6
(Assignee)

Comment 12

14 years ago
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.
(Assignee)

Updated

14 years ago
Attachment #164649 - Flags: review?(mkaply)

Comment 13

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

r=mkaply
Attachment #164649 - Flags: review?(mkaply) → review+
(Assignee)

Comment 14

14 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.