Message ID | CAJ+F1CLKFbu2sqz5WyC66naucmHqFfdT5Jg0e_uRmP0AGgJFNA@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 */ [...]
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 */ "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'; 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; } -- 2.5.0