Re: Patch for bug 25063 - _pclose should wait for the command processor to terminate and return it's exit status

[Borut Razem]

On 11/16/2010 01:56 PM, Alexandre Julliard wrote:
> Borut Razem  writes:
> > +static void msvcrt_add_handle(MSVCRT_FILE *fp, HANDLE *hProcess)
> > +{
> > - LOCK_POPEN;
> > - if (fp2handle == NULL)
> > - {
> > - /* array not allocated yet: allocate it */
> > - if ((fp2handle = MSVCRT_malloc(sizeof (struct fp2handle_s) * 16)) != NULL)
> > - {
> > -
> >  /* write handles to the first slot */
> > -
> >  fp2handle[0].fp = fp;
> > -
> >  fp2handle[0].hProcess = hProcess;
> > -
> > -
> >  /* zero the remaining space */
> > -
> >  memset(&fp2handle[1], 0, sizeof (struct fp2handle_s) * (16 - 1));
> > -
> > -
> >  /* update the number of slots */
> > -
> >  fp2handle_entries = 16;
> > - }
> > - }
> > - else
> > - {
> > - int i;
> > -
> > - /* search for free or slot with the same fp */
> > - for (i = 0; i<  fp2handle_entries; ++i)
> > - {
> > -
> >  if (fp2handle[i].fp == NULL || fp2handle[i].fp == fp)
> > -
> >  {
> > -
> >    /* found: write handles to the slot */
> > -
> >    fp2handle[i].fp = fp;
> > -
> >    fp2handle[i].hProcess = hProcess;
> > -
> >    break;
> > -
> >  }
> > - }
> > - if (i>= fp2handle_entries)
> > - {
> > -
> >  /* no space left: double the array size */
> > -
> >  struct fp2handle_s *newFp2handle;
> > -
> > -
> >  if ((newFp2handle = MSVCRT_realloc(fp2handle, sizeof (struct fp2handle_s) * (fp2handle_entries * 2))) != NULL)
> > -
> >  {
> > -
> >    fp2handle = newFp2handle;
> > -
> > -
> >    /* write handles to the first exetended slot */
> > -
> >    fp2handle[fp2handle_entries].fp = fp;
> > -
> >    fp2handle[fp2handle_entries].hProcess = hProcess;
> > -
> > -
> >    /* zero the remaining extended space */
> > -
> >    memset(&fp2handle[fp2handle_entries + 1], 0, sizeof (struct fp2handle_s) * (fp2handle_entries - 1));
> > -
> > -
> >    /* update the number of slots */
> > -
> >    fp2handle_entries *= 2;
> > -
> >  }
> > - }
> > - }
> > - UNLOCK_POPEN;
> >
> > +}
>
> That's an awful lot of code for such a simple thing. You probably want
> to use an array indexed by file descriptor or something like that. Also
> please don't add a comment before every line stating what the line
> does, that should be clear from the code itself.

Yes, it looks huge, but we can reduce by a third by removing empty lines 
and comments (just kidding ;-). But the run-time overhead is minimal: 
allocation of the array at the first popen call and reallocation if more 
then 16 childs are popened, which will probably happen very rarely. When 
the array is allocated, the cost is the same as in case of statically 
allocated array (OK, one additional "if" for initialization).

I implemented it as flexible as possible so that there are no 
limitations about the number of popened childs and no overhead in data 
space used by statically allocated arrays. In file.c there is a 
statically allocated array of file descriptors with 2048 elements, which 
seems a big overhead for popened childs: usually there are only few 
childs popened per process. OTOH there is a comment in file.c saying: /* 
FIXME: this should be allocated dynamically */, which is exactly what I 
did ;-)

If you still think that we should use a static array, just let me know, 
I'll re-implement it. And let me know what should be the array size.

Borut