On 02/14/2013 10:15 AM, Peter Krempa wrote:
> On 02/14/13 17:42, John Ferlan wrote:
>> ---
>> tests/commandtest.c | 17 ++++++++++++-----
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/commandtest.c b/tests/commandtest.c
>> index 93c6333..3bfc358 100644
>> --- a/tests/commandtest.c
>> +++ b/tests/commandtest.c
>
> ...
>
>> @@ -963,16 +964,19 @@ mymain(void)
>> dup2(fd, 6) < 0 ||
>> dup2(fd, 7) < 0 ||
>> dup2(fd, 8) < 0 ||
>> - (fd > 8 && VIR_CLOSE(fd) < 0))
>> + (fd > 8 && VIR_CLOSE(fd) < 0)) {
>> + VIR_FORCE_CLOSE(fd);
>> return EXIT_FAILURE;
>> + }
>> + sa_assert(fd == -1);
>
> according to man of dup2():
> * If oldfd is a valid file descriptor, and newfd has the same value
> as oldfd, then dup2() does nothing, and returns newfd.
>
> It is possible that open returns fd < 8, dup2() on that does nothing and
> afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at
this point. There are two cases to consider:
1. start life with fds 3-8 already opened by inheritance (rare). Opening
/dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was
inherited from the environment), then we close 9. If we fail to dup2,
we still close 9, but none of the other fds which we forcefully
overwrote. In this case, fd==-1 is true when you get to the sa_assert.
2. start life with at least one of fd 3-8 closed (most common). Opening
/dev/null then gets the first open fd, which we then dup to all the
remaining fds between 3-8. We don't close fd in this case, because we
want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
Does the assert actually fix anything? I think it is best to remove it.
>
>>
>> /* Prime the debug/verbose settings from the env vars,
>> * since we're about to reset 'environ' */
>> ignore_value(virTestGetDebug());
>> ignore_value(virTestGetVerbose());
>>
>> - if (virInitialize() < 0)
>> - return EXIT_FAILURE;
>> + /* Make sure to not leak fd's */
>> + virinitret = virInitialize();
>>
>> /* Phase two of killing interfering fds; see above. */
>> fd = 3;
>
> ACK with the assertion removed or a sufficient explanation provided.
>
> Peter
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list