Mail Archives: djgpp-workers/1997/11/26/14:03:33
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
- Raw text -