Re: [Qemu-devel] [PATCH v2 05/42] ivshmem-server: Don't overload POSIX shmem and file name

[Markus Armbruster]

Marc-André Lureau <address@hidden> writes:

> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <address@hidden> 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 <address@hidden>
>> ---
>
> 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 <address@hidden>

Thanks!

[...]
> From e8112678496fd873ceaa34b3169e516130075ed4 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <address@hidden>
> 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 <address@hidden>
> ---
>  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;
>      }