[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/4] Add basic version of bridge helper
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/4] Add basic version of bridge helper |
Date: |
Tue, 1 Nov 2011 08:15:14 +0000 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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.
> + /* 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().
> +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.
Stefan
- Re: [Qemu-devel] [PATCH v3 1/4] Add basic version of bridge helper,
Stefan Hajnoczi <=