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
