[v2,05/42] ivshmem-server: Don't overload POSIX shmem and file name

Hi

On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Option -m NAME is interpreted as directory name if we can statfs() it
> and its on hugetlbfs.  Else it's interpreted as POSIX shared memory
> object name.  This is nuts.
>
> Always interpret -m as directory.  Create new -M for POSIX shared
> memory.  Last of -m or -M wins.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

I don't see why the last should win is a good idea, see attached patch
for a possible solution, also changing a few comments. Feel free to
squash it in this patch or include it in your series.

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>



>  contrib/ivshmem-server/ivshmem-server.c | 56 ++++++---------------------------
>  contrib/ivshmem-server/ivshmem-server.h |  4 ++-
>  contrib/ivshmem-server/main.c           | 15 +++++++--
>  tests/ivshmem-test.c                    |  2 +-
>  4 files changed, 27 insertions(+), 50 deletions(-)
>
> diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
> index bfd0fad..41aee35 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -12,9 +12,6 @@
>  #include <sys/mman.h>
>  #include <sys/socket.h>
>  #include <sys/un.h>
> -#ifdef CONFIG_LINUX
> -#include <sys/vfs.h>
> -#endif
>
>  #include "ivshmem-server.h"
>
> @@ -257,7 +254,8 @@ ivshmem_server_ftruncate(int fd, unsigned shmsize)
>  /* Init a new ivshmem server */
>  int
>  ivshmem_server_init(IvshmemServer *server, const char *unix_sock_path,
> -                    const char *shm_path, size_t shm_size, unsigned n_vectors,
> +                    const char *shm_path, bool use_shm_open,
> +                    size_t shm_size, unsigned n_vectors,
>                      bool verbose)
>  {
>      int ret;
> @@ -278,6 +276,7 @@ ivshmem_server_init(IvshmemServer *server, const char *unix_sock_path,
>          return -1;
>      }
>
> +    server->use_shm_open = use_shm_open;
>      server->shm_size = shm_size;
>      server->n_vectors = n_vectors;
>
> @@ -286,31 +285,6 @@ ivshmem_server_init(IvshmemServer *server, const char *unix_sock_path,
>      return 0;
>  }
>
> -#ifdef CONFIG_LINUX
> -
> -#define HUGETLBFS_MAGIC       0x958458f6
> -
> -static long gethugepagesize(const char *path)
> -{
> -    struct statfs fs;
> -    int ret;
> -
> -    do {
> -        ret = statfs(path, &fs);
> -    } while (ret != 0 && errno == EINTR);
> -
> -    if (ret != 0) {
> -        return -1;
> -    }
> -
> -    if (fs.f_type != HUGETLBFS_MAGIC) {
> -        return -1;
> -    }
> -
> -    return fs.f_bsize;
> -}
> -#endif
> -
>  /* open shm, create and bind to the unix socket */
>  int
>  ivshmem_server_start(IvshmemServer *server)
> @@ -319,27 +293,17 @@ ivshmem_server_start(IvshmemServer *server)
>      int shm_fd, sock_fd, ret;
>
>      /* open shm file */
> -#ifdef CONFIG_LINUX
> -    long hpagesize;
> -
> -    hpagesize = gethugepagesize(server->shm_path);
> -    if (hpagesize < 0 && errno != ENOENT) {
> -        IVSHMEM_SERVER_DEBUG(server, "cannot stat shm file %s: %s\n",
> -                             server->shm_path, strerror(errno));
> -    }
> -
> -    if (hpagesize > 0) {
> +    if (server->use_shm_open) {
> +        IVSHMEM_SERVER_DEBUG(server, "Using POSIX shared memory: %s\n",
> +                             server->shm_path);
> +        shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
> +    } else {
>          gchar *filename = g_strdup_printf("%s/ivshmem.XXXXXX", server->shm_path);
> -        IVSHMEM_SERVER_DEBUG(server, "Using hugepages: %s\n", server->shm_path);
> +        IVSHMEM_SERVER_DEBUG(server, "Using file-backed shared memory: %s\n",
> +                             server->shm_path);
>          shm_fd = mkstemp(filename);
>          unlink(filename);
>          g_free(filename);
> -    } else
> -#endif
> -    {
> -        IVSHMEM_SERVER_DEBUG(server, "Using POSIX shared memory: %s\n",
> -                             server->shm_path);
> -        shm_fd = shm_open(server->shm_path, O_CREAT|O_RDWR, S_IRWXU);
>      }
>
>      if (shm_fd < 0) {
> diff --git a/contrib/ivshmem-server/ivshmem-server.h b/contrib/ivshmem-server/ivshmem-server.h
> index e9de8a3..3851639 100644
> --- a/contrib/ivshmem-server/ivshmem-server.h
> +++ b/contrib/ivshmem-server/ivshmem-server.h
> @@ -66,6 +66,7 @@ typedef struct IvshmemServer {
>      char unix_sock_path[PATH_MAX];   /**< path to unix socket */
>      int sock_fd;                     /**< unix sock file descriptor */
>      char shm_path[PATH_MAX];         /**< path to shm */
> +    bool use_shm_open;
>      size_t shm_size;                 /**< size of shm */
>      int shm_fd;                      /**< shm file descriptor */
>      unsigned n_vectors;              /**< number of vectors */
> @@ -89,7 +90,8 @@ typedef struct IvshmemServer {
>   */
>  int
>  ivshmem_server_init(IvshmemServer *server, const char *unix_sock_path,
> -                    const char *shm_path, size_t shm_size, unsigned n_vectors,
> +                    const char *shm_path, bool use_shm_open,
> +                    size_t shm_size, unsigned n_vectors,
>                      bool verbose);
>
>  /**
> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
> index e9b4388..2795db5 100644
> --- a/contrib/ivshmem-server/main.c
> +++ b/contrib/ivshmem-server/main.c
> @@ -29,6 +29,7 @@ typedef struct IvshmemServerArgs {
>      const char *pid_file;
>      const char *unix_socket_path;
>      const char *shm_path;
> +    bool use_shm_open;
>      uint64_t shm_size;
>      unsigned n_vectors;
>  } IvshmemServerArgs;
> @@ -44,8 +45,9 @@ ivshmem_server_usage(const char *progname)
>             "     default " IVSHMEM_SERVER_DEFAULT_PID_FILE "\n"
>             "  -S <unix_socket_path>: path to the unix socket to listen to\n"
>             "     default " IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH "\n"
> -           "  -m <shm_path>: POSIX shared memory object name or a hugetlbfs mount point\n"
> +           "  -M <name>: POSIX shared memory object to use\n"
>             "     default " IVSHMEM_SERVER_DEFAULT_SHM_PATH "\n"
> +           "  -m <dirname>: where to create shared memory\n"
>             "  -l <size>: size of shared memory in bytes\n"
>             "     suffixes K, M and G can be used, e.g. 1K means 1024\n"
>             "     default %u\n"
> @@ -76,6 +78,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>                         "p:" /* pid_file */
>                         "S:" /* unix_socket_path */
>                         "m:" /* shm_path */
> +                       "M:" /* shm_path */
>                         "l:" /* shm_size */
>                         "n:" /* n_vectors */
>                        )) != -1) {
> @@ -102,8 +105,14 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>              args->unix_socket_path = optarg;
>              break;
>
> +        case 'M': /* shm_path */
> +            args->shm_path = optarg;
> +            args->use_shm_open = true;
> +            break;
> +
>          case 'm': /* shm_path */
>              args->shm_path = optarg;
> +            args->use_shm_open = false;
>              break;
>
>          case 'l': /* shm_size */
> @@ -199,6 +208,7 @@ main(int argc, char *argv[])
>          .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
>          .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
>          .shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
> +        .use_shm_open = true,
>          .shm_size = IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
>          .n_vectors = IVSHMEM_SERVER_DEFAULT_N_VECTORS,
>      };
> @@ -226,7 +236,8 @@ main(int argc, char *argv[])
>      }
>
>      /* init the ivshms structure */
> -    if (ivshmem_server_init(&server, args.unix_socket_path, args.shm_path,
> +    if (ivshmem_server_init(&server, args.unix_socket_path,
> +                            args.shm_path, args.use_shm_open,
>                              args.shm_size, args.n_vectors, args.verbose) < 0) {
>          fprintf(stderr, "cannot init server\n");
>          goto err;
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index e184c67..4efa433 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -294,7 +294,7 @@ static void test_ivshmem_server(bool msi)
>      guint64 end_time = g_get_monotonic_time() + 5 * G_TIME_SPAN_SECOND;
>
>      memset(tmpshmem, 0x42, TMPSHMSIZE);
> -    ret = ivshmem_server_init(&server, tmpserver, tmpshm,
> +    ret = ivshmem_server_init(&server, tmpserver, tmpshm, true,
>                                TMPSHMSIZE, nvectors,
>                                g_test_verbose());
>      g_assert_cmpint(ret, ==, 0);
> --
> 2.4.3
>
>

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Option -m NAME is interpreted as directory name if we can statfs() it
>> and its on hugetlbfs.  Else it's interpreted as POSIX shared memory
>> object name.  This is nuts.
>>
>> Always interpret -m as directory.  Create new -M for POSIX shared
>> memory.  Last of -m or -M wins.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
> I don't see why the last should win is a good idea, see attached patch

Last one wins is pretty common behavior.  In fact, it's what this
program does for every single option with an argument.  I didn't feel
like making -m and -M special.

> for a possible solution, also changing a few comments. Feel free to
> squash it in this patch or include it in your series.

I got a few comments inline.

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Thanks!

[...]
> From e8112678496fd873ceaa34b3169e516130075ed4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@redhat.com>
> Date: Tue, 8 Mar 2016 20:31:09 +0100
> Subject: [PATCH] ivshmem-server: expect either -m or -M
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  contrib/ivshmem-server/main.c | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/contrib/ivshmem-server/main.c b/contrib/ivshmem-server/main.c
> index 2795db5..368fc67 100644
> --- a/contrib/ivshmem-server/main.c
> +++ b/contrib/ivshmem-server/main.c
> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>                         "F"  /* foreground */
>                         "p:" /* pid_file */
>                         "S:" /* unix_socket_path */
> -                       "m:" /* shm_path */
> +                       "m:" /* dirname */

The existing comments all name the member of args set by the option.
There is no member dirname.

>                         "M:" /* shm_path */
>                         "l:" /* shm_size */
>                         "n:" /* n_vectors */
> @@ -106,13 +106,15 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>              break;
>  
>          case 'M': /* shm_path */
> -            args->shm_path = optarg;
> -            args->use_shm_open = true;
> -            break;
> +        case 'm': /* dirname */
> +            if (args->shm_path) {
> +                fprintf(stderr, "Please specify either -m or -M.\n");
> +                ivshmem_server_help(argv[0]);
> +                exit(1);
> +            }
>  
> -        case 'm': /* shm_path */
>              args->shm_path = optarg;
> -            args->use_shm_open = false;
> +            args->use_shm_open = c == 'M';

I think I'll steal this idea :)

>              break;
>  
>          case 'l': /* shm_size */
> @@ -207,7 +209,7 @@ main(int argc, char *argv[])
>          .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
>          .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
>          .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
> -        .shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
> +        .shm_path = NULL,
>          .use_shm_open = true,
>          .shm_size = IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
>          .n_vectors = IVSHMEM_SERVER_DEFAULT_N_VECTORS,
> @@ -237,8 +239,9 @@ main(int argc, char *argv[])
>  
>      /* init the ivshms structure */
>      if (ivshmem_server_init(&server, args.unix_socket_path,
> -                            args.shm_path, args.use_shm_open,
> -                            args.shm_size, args.n_vectors, args.verbose) < 0) {
> +                            args.shm_path ?: IVSHMEM_SERVER_DEFAULT_SHM_PATH,
> +                            args.use_shm_open, args.shm_size, args.n_vectors,
> +                            args.verbose) < 0) {
>          fprintf(stderr, "cannot init server\n");
>          goto err;
>      }

Hi

On Wed, Mar 9, 2016 at 9:14 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>>                         "F"  /* foreground */
>>                         "p:" /* pid_file */
>>                         "S:" /* unix_socket_path */
>> -                       "m:" /* shm_path */
>> +                       "m:" /* dirname */
>
> The existing comments all name the member of args set by the option.
> There is no member dirname.

I read from help: "-m <dirname>: where to create shared memory"

>
>>                         "M:" /* shm_path */
>>                         "l:" /* shm_size */
>>                         "n:" /* n_vectors */
>> @@ -106,13 +106,15 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>>              break;
>>
>>          case 'M': /* shm_path */
>> -            args->shm_path = optarg;
>> -            args->use_shm_open = true;
>> -            break;
>> +        case 'm': /* dirname */
>> +            if (args->shm_path) {
>> +                fprintf(stderr, "Please specify either -m or -M.\n");
>> +                ivshmem_server_help(argv[0]);
>> +                exit(1);
>> +            }
>>
>> -        case 'm': /* shm_path */
>>              args->shm_path = optarg;
>> -            args->use_shm_open = false;
>> +            args->use_shm_open = c == 'M';
>
> I think I'll steal this idea :)

feel free

>
>>              break;
>>
>>          case 'l': /* shm_size */
>> @@ -207,7 +209,7 @@ main(int argc, char *argv[])
>>          .foreground = IVSHMEM_SERVER_DEFAULT_FOREGROUND,
>>          .pid_file = IVSHMEM_SERVER_DEFAULT_PID_FILE,
>>          .unix_socket_path = IVSHMEM_SERVER_DEFAULT_UNIX_SOCK_PATH,
>> -        .shm_path = IVSHMEM_SERVER_DEFAULT_SHM_PATH,
>> +        .shm_path = NULL,
>>          .use_shm_open = true,
>>          .shm_size = IVSHMEM_SERVER_DEFAULT_SHM_SIZE,
>>          .n_vectors = IVSHMEM_SERVER_DEFAULT_N_VECTORS,
>> @@ -237,8 +239,9 @@ main(int argc, char *argv[])
>>
>>      /* init the ivshms structure */
>>      if (ivshmem_server_init(&server, args.unix_socket_path,
>> -                            args.shm_path, args.use_shm_open,
>> -                            args.shm_size, args.n_vectors, args.verbose) < 0) {
>> +                            args.shm_path ?: IVSHMEM_SERVER_DEFAULT_SHM_PATH,
>> +                            args.use_shm_open, args.shm_size, args.n_vectors,
>> +                            args.verbose) < 0) {
>>          fprintf(stderr, "cannot init server\n");
>>          goto err;
>>      }

thanks

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Wed, Mar 9, 2016 at 9:14 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>> @@ -77,7 +77,7 @@ ivshmem_server_parse_args(IvshmemServerArgs *args, int argc, char *argv[])
>>>                         "F"  /* foreground */
>>>                         "p:" /* pid_file */
>>>                         "S:" /* unix_socket_path */
>>> -                       "m:" /* shm_path */
>>> +                       "m:" /* dirname */
>>
>> The existing comments all name the member of args set by the option.
>> There is no member dirname.
>
> I read from help: "-m <dirname>: where to create shared memory"

Differently logical.  In your interpretation, the comments are of very
little value.  In mine, even less.  That makes yours "superior".

>>>                         "M:" /* shm_path */
>>>                         "l:" /* shm_size */
>>>                         "n:" /* n_vectors */
[...]