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

[Corey Bryant]

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