Closed Bug 265219 Opened 21 years ago Closed 21 years ago

No support for redirecting stdin/stdout of a child process

Categories

(NSPR :: NSPR, defect)

x86
OS/2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: davide, Assigned: wtc)

Details

Attachments

(2 files, 3 obsolete files)

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
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.
Attachment #162695 - Attachment filename: os2misc.diff
Attachment #162695 - Attachment is obsolete: true
Great! I will take a closer look at the patch and try it out, hopefully tonight.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
(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?
Attachment #162701 - Attachment is obsolete: true
(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).
Attachment #162989 - Flags: superreview?(wchang0222)
Attachment #162989 - Flags: review?(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.
Modified os2misc.diff, removed useless things
Attachment #162989 - Attachment is obsolete: true
Attachment #162989 - Flags: superreview?(wchang0222)
Attachment #162989 - Flags: review?(mkaply)
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+
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+
Status: NEW → ASSIGNED
Target Milestone: --- → 4.6
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.
Attachment #164649 - Flags: review?(mkaply)
Comment on attachment 164649 [details] [diff] [review] Go to errorExit on DosStartSession failure r=mkaply
Attachment #164649 - Flags: review?(mkaply) → review+
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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: