Patch for bug 25063 - _pclose should wait for the command processor to terminate and return it's exit status
Borut Razem
borut.razem at siol.net
Tue Nov 16 13:13:02 CST 2010
On 11/16/2010 01:56 PM, Alexandre Julliard wrote:
> Borut Razem<borut.razem at siol.net> 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
More information about the wine-devel
mailing list