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
