Sender: ml AT delorie DOT com
Message-ID: <347C716B.36D94240@cdata.tvnet.hu>
Date: Wed, 26 Nov 1997 19:58:51 +0100
From: Molnar Laszlo <molnarl AT cdata DOT tvnet DOT hu>
MIME-Version: 1.0
To: Eli Zaretskii <eliz AT is DOT elta DOT co DOT il>
CC: djgpp-workers AT delorie DOT com
Subject: Re: popen/pclose update
References: <Pine DOT SUN DOT 3 DOT 91 DOT 971126131540 DOT 960B-100000 AT is>
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

Eli Zaretskii wrote:
> 
> > 2, l1->temp_name might not be freed when something failed
> In the original code, l1->temp_name is not malloc'ed, so it doesn't
> need to be freed.

Oops, sorry.

> > 5, pipe_list.fd is not needed.
> So?

Why not remove then?

> > Some problems with pclose():
> >
> > 1, It fails when it is called with a not popen-ed pointer (broken and
> >    too complex list handling)
> 
> I think it crashes because it unconditionally dereferences pl, and
> only if no matching call to `popen' was done.  Nothing a simple patch
> (below) can't fix.

Well, this patch is not enough. See why:

	FILE *f1,*f2;
	f1=popen ("ls","r");
	f2=popen ("ls","r");
	pclose (stdin);

This will give a SIGSEGV, so pclose needs another 2 lines. At this point
I think my solution with 'for' is better.

> > Patching these one by one would require changing at least 30-40 lines,
> > that's why I thought that I should made a general cleanup.
> The patch below adds 15 lines to the original version and deletes
> one.

Ok, if you like spaghetti code...

> > My reimplementation does the same things as the original code when
> > everything works ok, but I try to handle the possible errors too.
> 
> Your code calls `strdup'.  You can't do that in `popen', because
> `popen' and `pclose' are POSIX functions whereas `strdup' is not.

Ok, I'll resend my code tomorrow to DJ.

> > Reorganizing the code also made it shorter (~80 lines vs ~120 lines)
> > and cleaner (IMHO). And the autodeleting feature of the temporary
> > files is just 3 extra lines.
> 
> I will let DJ decide whether to accept your version (after you replace
> the calls to `strdup') or the patch below.

Hey, I really didn't want a contention :-) My version is just a
suggestion, because I thought this code was a little buggy, and I could
improve it. If you don't want it, that's no problem. But the
autodeleting feature is useful for perl, so I've put this version in it.

Laszlo