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)
Tracking
(Not tracked)
RESOLVED
FIXED
4.6
People
(Reporter: davide, Assigned: wtc)
Details
Attachments
(2 files, 3 obsolete files)
4.12 KB,
patch
|
mkaply
:
review+
wtc
:
superreview+
|
Details | Diff | Splinter Review |
637 bytes,
patch
|
mkaply
:
review+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
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•21 years ago
|
Attachment #162695 -
Attachment filename: os2misc.diff
Reporter | ||
Comment 2•21 years ago
|
||
Attachment #162695 -
Attachment is obsolete: true
Comment 3•21 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•21 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•21 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•21 years ago
|
||
Attachment #162701 -
Attachment is obsolete: true
Comment 7•21 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).
Attachment #162989 -
Flags: superreview?(wchang0222)
Attachment #162989 -
Flags: review?(mkaply)
Comment 8•21 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•21 years ago
|
||
Modified os2misc.diff, removed useless things
Reporter | ||
Updated•21 years ago
|
Attachment #162989 -
Attachment is obsolete: true
Updated•21 years ago
|
Attachment #162989 -
Flags: superreview?(wchang0222)
Attachment #162989 -
Flags: review?(mkaply)
Comment 10•21 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•21 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•21 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → 4.6
Assignee | ||
Comment 12•21 years ago
|
||
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•21 years ago
|
Attachment #164649 -
Flags: review?(mkaply)
Comment 13•21 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•21 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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•