Re: [PATCH] Fix a typo

[Eric Blake]

On 03/28/2011 05:41 AM, Bastien ROUCARIES wrote:
> Fix a typo in passfd code
> ---
>  lib/passfd.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/lib/passfd.c b/lib/passfd.c
> index 573b80e..ae716a6 100644
> --- a/lib/passfd.c
> +++ b/lib/passfd.c
> @@ -67,7 +67,6 @@ sendfd (int sock, int fd)
>      cmsg->cmsg_len = CMSG_LEN (sizeof (int));
>      /* Initialize the payload: */
>      memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd));
> -    msg.msg_controllen = cmsg->cmsg_len;
>  #elif HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY
>      msg.msg_accrights = &fd;
>      msg.msg_accrightslen = sizeof (fd);

Hmm, both before and after this patch of yours, I get things to compile
on NetBSD 5.0.2, but the test hangs in both cases, after printing:

sendfd: Invalid argument

Hanging a unit test is bad; so I'm also going to check in a followup
that does an alarm(5) to the self-test (we've done it elsewhere, so I
have precedence).  However, I also concur that your patch is reasonable,
as you already assigned msg_controllen earlier in the function.  I
guessed that the reason for the failure is that you forgot to assign
msg_flags, and NetBSD was rejecting uninitialized data as invalid flags.
 However, after pushing this patch, things still fail, so I'm still
investigating.

From cc0745fc8e17997b71a1e021ca15c0b95ba779b9 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 30 Mar 2011 14:46:02 -0600
Subject: [PATCH] passfd: fix incorrect sendmsg arguments

The unit test hung on NetBSD, which pointed out a couple of bugs.

* lib/passfd.c (sendfd): Avoid uninitialized msg_flags field, and
incorrect msg_controllen value.
* modules/passfd-tests (Depends-on): Check for alarm.
* tests/test-passfd.c (main) [HAVE_DECL_ALARM]: Avoid hanging test.
Reported by Bastien ROUCARIES.
---
 ChangeLog            |    9 +++++++++
 lib/passfd.c         |    2 +-
 modules/passfd-tests |    1 +
 tests/test-passfd.c  |    7 +++++++
 4 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 09a5810..ec324a0 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2011-03-30  Eric Blake  <address@hidden>
+
+       passfd: fix incorrect sendmsg arguments
+       * lib/passfd.c (sendfd): Avoid uninitialized msg_flags field, and
+       incorrect msg_controllen value.
+       * modules/passfd-tests (Depends-on): Check for alarm.
+       * tests/test-passfd.c (main) [HAVE_DECL_ALARM]: Avoid hanging test.
+       Reported by Bastien ROUCARIES.
+
 2011-03-30  Jim Meyering  <address@hidden>

        tests: readlink* ("",... fails with EINVAL on newer kernels
diff --git a/lib/passfd.c b/lib/passfd.c
index 573b80e..5bf400d 100644
--- a/lib/passfd.c
+++ b/lib/passfd.c
@@ -47,6 +47,7 @@ sendfd (int sock, int fd)
   struct msghdr msg;

   /* send at least one char */
+  memset (&msg, 0, sizeof msg);
   iov[0].iov_base = &send;
   iov[0].iov_len = 1;
   msg.msg_iov = iov;
@@ -67,7 +68,6 @@ sendfd (int sock, int fd)
     cmsg->cmsg_len = CMSG_LEN (sizeof (int));
     /* Initialize the payload: */
     memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd));
-    msg.msg_controllen = cmsg->cmsg_len;
 #elif HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY
     msg.msg_accrights = &fd;
     msg.msg_accrightslen = sizeof (fd);
diff --git a/modules/passfd-tests b/modules/passfd-tests
index 132679c..477754b 100644
--- a/modules/passfd-tests
+++ b/modules/passfd-tests
@@ -5,6 +5,7 @@ tests/macros.h
 Depends-on:

 configure.ac:
+AC_CHECK_DECLS_ONCE([alarm])

 Makefile.am:
 TESTS += test-passfd
diff --git a/tests/test-passfd.c b/tests/test-passfd.c
index 2048934..d657ad9 100644
--- a/tests/test-passfd.c
+++ b/tests/test-passfd.c
@@ -19,6 +19,7 @@
 #include "passfd.h"

 #include <fcntl.h>
+#include <signal.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>
@@ -40,6 +41,12 @@ main ()
   int fd;
   struct stat st;

+#if HAVE_DECL_ALARM
+  /* Avoid hanging on failure.  */
+  signal (SIGALRM, SIG_DFL);
+  alarm (5);
+#endif
+
   fdnull = open ("/dev/null", O_RDWR);
   if (fdnull < 0)
     {
-- 
1.7.4



I'm also pushing this followup, which reduces the code size a bit (we
prefer sizeof variable over sizeof (type), NULL over 0 for null
pointers, there's no need to make a one-element array, and we prefer to
avoid in-function #ifdefs when possible).

From aeea9482713470cd1e4cd78b39c8d87841c62b2f Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Wed, 30 Mar 2011 15:30:13 -0600
Subject: [PATCH] passfd: standardize coding conventions

* m4/afunix.m4 (gl_SOCKET_AFUNIX): Drop check for something that
can be learned at compile time.
* lib/passfd.c (MSG_CMSG_CLOEXEC): Reduce number of in-function
ifdefs.
(passfd, recvfd): Follow gnulib code conventions.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog    |    7 +++++++
 lib/passfd.c |   48 +++++++++++++++++++++++-------------------------
 m4/afunix.m4 |   29 +----------------------------
 3 files changed, 31 insertions(+), 53 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ec324a0..5d4dc84 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@
 2011-03-30  Eric Blake  <address@hidden>

+       passfd: standardize coding conventions
+       * m4/afunix.m4 (gl_SOCKET_AFUNIX): Drop check for something that
+       can be learned at compile time.
+       * lib/passfd.c (MSG_CMSG_CLOEXEC): Reduce number of in-function
+       ifdefs.
+       (passfd, recvfd): Follow gnulib code conventions.
+
        passfd: fix incorrect sendmsg arguments
        * lib/passfd.c (sendfd): Avoid uninitialized msg_flags field, and
        incorrect msg_controllen value.
diff --git a/lib/passfd.c b/lib/passfd.c
index 5bf400d..f507dde 100644
--- a/lib/passfd.c
+++ b/lib/passfd.c
@@ -34,6 +34,10 @@

 #include "cloexec.h"

+#ifndef MSG_CMSG_CLOEXEC
+# define MSG_CMSG_CLOEXEC 0
+#endif
+
 /* sendfd sends the file descriptor fd along the socket
    to a process calling recvfd on the other end.

@@ -43,41 +47,41 @@ int
 sendfd (int sock, int fd)
 {
   char send = 0;
-  struct iovec iov[1];
+  struct iovec iov;
   struct msghdr msg;

   /* send at least one char */
   memset (&msg, 0, sizeof msg);
-  iov[0].iov_base = &send;
-  iov[0].iov_len = 1;
-  msg.msg_iov = iov;
+  iov.iov_base = &send;
+  iov.iov_len = 1;
+  msg.msg_iov = &iov;
   msg.msg_iovlen = 1;
-  msg.msg_name = 0;
+  msg.msg_name = NULL;
   msg.msg_namelen = 0;

   {
 #if HAVE_UNIXSOCKET_SCM_RIGHTS_BSD44_WAY
     struct cmsghdr *cmsg;
-    char buf[CMSG_SPACE (sizeof (fd))];
+    char buf[CMSG_SPACE (sizeof fd)];

     msg.msg_control = buf;
-    msg.msg_controllen = sizeof (buf);
+    msg.msg_controllen = sizeof buf;
     cmsg = CMSG_FIRSTHDR (&msg);
     cmsg->cmsg_level = SOL_SOCKET;
     cmsg->cmsg_type = SCM_RIGHTS;
-    cmsg->cmsg_len = CMSG_LEN (sizeof (int));
+    cmsg->cmsg_len = CMSG_LEN (sizeof fd);
     /* Initialize the payload: */
-    memcpy (CMSG_DATA (cmsg), &fd, sizeof (fd));
+    memcpy (CMSG_DATA (cmsg), &fd, sizeof fd);
 #elif HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY
     msg.msg_accrights = &fd;
-    msg.msg_accrightslen = sizeof (fd);
+    msg.msg_accrightslen = sizeof fd;
 #else
     errno = ENOSYS;
     return -1;
 #endif
   }

-  if (sendmsg (sock, &msg, 0) != iov[0].iov_len)
+  if (sendmsg (sock, &msg, 0) != iov.iov_len)
     return -1;
   return 0;
 }
@@ -91,7 +95,7 @@ int
 recvfd (int sock, int flags)
 {
   char recv = 0;
-  struct iovec iov[1];
+  struct iovec iov;
   struct msghdr msg;

   if ((flags & ~O_CLOEXEC) != 0)
@@ -101,24 +105,20 @@ recvfd (int sock, int flags)
     }

   /* send at least one char */
-  iov[0].iov_base = &recv;
-  iov[0].iov_len = 1;
-  msg.msg_iov = iov;
+  iov.iov_base = &recv;
+  iov.iov_len = 1;
+  msg.msg_iov = &iov;
   msg.msg_iovlen = 1;
-  msg.msg_name = 0;
+  msg.msg_name = NULL;
   msg.msg_namelen = 0;

   {
 #if HAVE_UNIXSOCKET_SCM_RIGHTS_BSD44_WAY
     int fd;
     struct cmsghdr *cmsg;
-    char buf[CMSG_SPACE (sizeof (fd))];
+    char buf[CMSG_SPACE (sizeof fd)];
     const int mone = -1;
-# if HAVE_MSG_CMSG_CLOEXEC
-    int flags_recvmsg = (flags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0);
-# else
-    int flags_recvmsg = 0;
-# endif
+    int flags_recvmsg = flags & O_CLOEXEC ? MSG_CMSG_CLOEXEC : 0;

     msg.msg_control = buf;
     msg.msg_controllen = sizeof (buf);
@@ -145,9 +145,8 @@ recvfd (int sock, int flags)

     memcpy (&fd, CMSG_DATA (cmsg), sizeof (fd));

-# if !HAVE_MSG_CMSG_CLOEXEC
     /* set close-on-exec flag */
-    if (flags & O_CLOEXEC)
+    if (!MSG_CMSG_CLOEXEC && (flags & O_CLOEXEC))
       {
         if (set_cloexec_flag (fd, true) < 0)
           {
@@ -157,7 +156,6 @@ recvfd (int sock, int flags)
             return -1;
           }
       }
-# endif

     return fd;

diff --git a/m4/afunix.m4 b/m4/afunix.m4
index 46b3bf3..13f7583 100644
--- a/m4/afunix.m4
+++ b/m4/afunix.m4
@@ -1,4 +1,4 @@
-# afunix.m4 serial 5
+# afunix.m4 serial 6
 dnl Copyright (C) 2011 Free Software Foundation, Inc.
 dnl This file is free software; the Free Software Foundation
 dnl gives unlimited permission to copy and/or distribute it,
@@ -113,31 +113,4 @@ AC_DEFUN([gl_SOCKET_AFUNIX],
     AC_DEFINE([HAVE_UNIXSOCKET_SCM_RIGHTS_BSD43_WAY], [1],
       [Define to 1 if fd can be sent/received in the BSD4.3 way.])
   fi
-
-  AC_MSG_CHECKING([for UNIX domain sockets recvmsg() MSG_CMSG_CLOEXEC
flag])
-  AC_CACHE_VAL([gl_cv_socket_unix_msg_cmsg_cloexec],
-    [AC_COMPILE_IFELSE(
-       [AC_LANG_PROGRAM(
-          [[#include <sys/types.h>
-            #ifdef HAVE_SYS_SOCKET_H
-            #include <sys/socket.h>
-            #endif
-            #ifdef HAVE_SYS_UN_H
-            #include <sys/un.h>
-            #endif
-            #ifdef HAVE_WINSOCK2_H
-            #include <winsock2.h>
-            #endif
-            ]],
-            [[int flags = MSG_CMSG_CLOEXEC;
-              if (&flags) return 0;
-            ]])],
-       [gl_cv_socket_unix_msg_cmsg_cloexec=yes],
-       [gl_cv_socket_unix_msg_cmsg_cloexec=no])
-    ])
-  AC_MSG_RESULT([$gl_cv_socket_unix_msg_cmsg_cloexec])
-  if test $gl_cv_socket_unix_msg_cmsg_cloexec = yes; then
-    AC_DEFINE([HAVE_MSG_CMSG_CLOEXEC], [1],
-      [Define to 1 if recvmsg could be specified with MSG_CMSG_CLOEXEC.])
-  fi
 ])
-- 
1.7.4



-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature