Re: [Qemu-devel] [PATCH v3 1/4] Add basic version of bridge helper

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 11/01/2011 04:15 AM, Stefan Hajnoczi wrote:
> On Mon, Oct 31, 2011 at 02:36:28PM -0400, Corey Bryant wrote:
> A couple of nitpicks regarding error handling:
>
>> +static int has_vnet_hdr(int fd)
>> +{
>> +    unsigned int features = 0;
>> +    struct ifreq ifreq;
>> +
>> +    if (ioctl(fd, TUNGETFEATURES,&features) == -1) {
>> +        return -errno;
>> +    }
>> +
>> +    if (!(features&  IFF_VNET_HDR)) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    if (ioctl(fd, TUNGETIFF,&ifreq) != -1 || errno != EBADFD) {
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    return 1;
>> +}
>
> This function is strange, it looks like a boolean function but actually
> only returns 1 or -errno.  It is used incorrectly in main().  I suggest
> changing the return value to bool and returning false on error.
>

Ah, good catch, this was a bug.  And I agree that bool would work 
better.  I'll fix this.

>> +    /* open a socket to use to control the network interfaces */
>> +    ctlfd = socket(AF_INET, SOCK_STREAM, 0);
>> +    if (ctlfd == -1) {
>> +        fprintf(stderr, "failed to open control socket\n");
>> +        ret = -errno;
>
> It's better to stash away errno before invoking other library functions.
> man errno(3) says:
>
> "a function that succeeds is allowed to change errno"
>
> This means fprintf(3) could clobber errno.
>
> I suggest simply printing out errno with the error message and returning
> exit code 1 (EXIT_FAILURE).  The same applies for the other error exit
> cases in main().
>

I agree.  I'll fix this.

>> +cleanup:
>> +
>> +    close(fd);
>> +
>> +    close(ctlfd);
>
> ctlfd is an uninitialized variable if opening fd fails.  We also never
> close unixfd.
>
> I'd remove this cleanup code and just return without closing any file
> descriptors - let the kernel do it.

Ok, I'll do this.  But I think I'll re-introduce the cleanup goto in 
patch 2/4 to free the simple queue memory.

-- 
Regards,
Corey


>
> Stefan
>