* [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket @ 2019-08-11 22:55 Masayoshi Mizuma 2019-08-12 3:12 ` piaojun 2019-08-13 15:51 ` Stefan Hajnoczi 0 siblings, 2 replies; 7+ messages in thread From: Masayoshi Mizuma @ 2019-08-11 22:55 UTC (permalink / raw) To: piaojun, Stefan Hajnoczi, virtio-fs; +Cc: Masayoshi Mizuma From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> virtiofsd can run multiply even if the vhost_user_socket is same path. ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & [1] 244965 virtio_session_mount: Waiting for vhost-user socket connection... ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & [2] 244966 virtio_session_mount: Waiting for vhost-user socket connection... ]# The user will get confused about the situation and maybe the cause of the unexpected problem. So it's better to prevent the multiple running. Create a regular file under localstatedir directory to exclude the vhost_user_socket. To create and lock the file, use qemu_write_pidfile() because the API has some sanity checks and file lock. Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> --- contrib/virtiofsd/fuse_i.h | 1 + contrib/virtiofsd/fuse_lowlevel.c | 3 ++ contrib/virtiofsd/fuse_virtio.c | 50 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/contrib/virtiofsd/fuse_i.h b/contrib/virtiofsd/fuse_i.h index ff6e1b75ba..d31c675469 100644 --- a/contrib/virtiofsd/fuse_i.h +++ b/contrib/virtiofsd/fuse_i.h @@ -72,6 +72,7 @@ struct fuse_session { char *vu_socket_path; int vu_listen_fd; int vu_socketfd; + char *vu_socket_lock_file; struct fv_VuDev *virtio_dev; }; diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c index 8adc4b1ab8..ab18b86435 100644 --- a/contrib/virtiofsd/fuse_lowlevel.c +++ b/contrib/virtiofsd/fuse_lowlevel.c @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) free(se->vu_socket_path); se->vu_socket_path = NULL; + unlink(se->vu_socket_lock_file); + free(se->vu_socket_lock_file); + free(se); } diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c index 083e4fc317..4999f95b63 100644 --- a/contrib/virtiofsd/fuse_virtio.c +++ b/contrib/virtiofsd/fuse_virtio.c @@ -32,6 +32,9 @@ #include "contrib/libvhost-user/libvhost-user.h" +#include "qemu/osdep.h" +#include "qapi/error.h" + struct fv_VuDev; struct fv_QueueInfo { pthread_t thread; @@ -862,6 +865,47 @@ int virtio_loop(struct fuse_session *se) return 0; } +static void strreplace(char *s, char old, char new) +{ + for (; *s; ++s) + if (*s == old) + *s = new; +} + +static int fv_create_socket_lock_file(struct fuse_session *se) +{ + char *dir, *socket_name; + Error *local_err = NULL; + + dir = qemu_get_local_state_pathname("run/virtiofsd"); + + if (g_mkdir_with_parents(dir, S_IRWXU) < -1) { + fuse_err("%s: Failed to create directory %s: %s", + __func__, dir, strerror(errno)); + return -1; + } + + socket_name = malloc(strlen(se->vu_socket_path) + 1); + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); + strreplace(socket_name, '/', '.'); + + se->vu_socket_lock_file = malloc(NAME_MAX); + memset(se->vu_socket_lock_file, 0, NAME_MAX); + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", + dir, socket_name); + + if (!qemu_write_pidfile(se->vu_socket_lock_file, &local_err)) { + error_report_err(local_err); + return -1; + } + + free(socket_name); + g_free(dir); + + return 0; +} + static int fv_create_listen_socket(struct fuse_session *se) { struct sockaddr_un un; @@ -876,6 +920,12 @@ static int fv_create_listen_socket(struct fuse_session *se) return -1; } + /* Check the vu_socket_path is already used */ + if (fv_create_socket_lock_file(se) == -1) { + fuse_err("%s: Socket lock file creation failed\n", __func__); + return -1; + } + /* Create the Unix socket to communicate with qemu * based on QEMU's vhost-user-bridge */ -- 2.18.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-11 22:55 [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket Masayoshi Mizuma @ 2019-08-12 3:12 ` piaojun 2019-08-12 19:02 ` Masayoshi Mizuma 2019-08-13 15:51 ` Stefan Hajnoczi 1 sibling, 1 reply; 7+ messages in thread From: piaojun @ 2019-08-12 3:12 UTC (permalink / raw) To: Masayoshi Mizuma, Stefan Hajnoczi, virtio-fs; +Cc: Masayoshi Mizuma Hi Masayoshi, On 2019/8/12 6:55, Masayoshi Mizuma wrote: > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > > virtiofsd can run multiply even if the vhost_user_socket is > same path. > > ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > [1] 244965 > virtio_session_mount: Waiting for vhost-user socket connection... > ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > [2] 244966 > virtio_session_mount: Waiting for vhost-user socket connection... > ]# > > The user will get confused about the situation and maybe the cause of the > unexpected problem. So it's better to prevent the multiple running. > > Create a regular file under localstatedir directory to exclude the > vhost_user_socket. To create and lock the file, use qemu_write_pidfile() > because the API has some sanity checks and file lock. > > Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > --- > contrib/virtiofsd/fuse_i.h | 1 + > contrib/virtiofsd/fuse_lowlevel.c | 3 ++ > contrib/virtiofsd/fuse_virtio.c | 50 +++++++++++++++++++++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/contrib/virtiofsd/fuse_i.h b/contrib/virtiofsd/fuse_i.h > index ff6e1b75ba..d31c675469 100644 > --- a/contrib/virtiofsd/fuse_i.h > +++ b/contrib/virtiofsd/fuse_i.h > @@ -72,6 +72,7 @@ struct fuse_session { > char *vu_socket_path; > int vu_listen_fd; > int vu_socketfd; > + char *vu_socket_lock_file; I prefer char *vu_socket_lock which is just a personal habit. > struct fv_VuDev *virtio_dev; > }; > > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c > index 8adc4b1ab8..ab18b86435 100644 > --- a/contrib/virtiofsd/fuse_lowlevel.c > +++ b/contrib/virtiofsd/fuse_lowlevel.c > @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) > free(se->vu_socket_path); > se->vu_socket_path = NULL; > > + unlink(se->vu_socket_lock_file); > + free(se->vu_socket_lock_file); > + > free(se); > } > > diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c > index 083e4fc317..4999f95b63 100644 > --- a/contrib/virtiofsd/fuse_virtio.c > +++ b/contrib/virtiofsd/fuse_virtio.c > @@ -32,6 +32,9 @@ > > #include "contrib/libvhost-user/libvhost-user.h" > > +#include "qemu/osdep.h" > +#include "qapi/error.h" > + > struct fv_VuDev; > struct fv_QueueInfo { > pthread_t thread; > @@ -862,6 +865,47 @@ int virtio_loop(struct fuse_session *se) > return 0; > } > > +static void strreplace(char *s, char old, char new) > +{ > + for (; *s; ++s) > + if (*s == old) > + *s = new; > +} > + > +static int fv_create_socket_lock_file(struct fuse_session *se) > +{ > + char *dir, *socket_name; > + Error *local_err = NULL; > + > + dir = qemu_get_local_state_pathname("run/virtiofsd"); > + > + if (g_mkdir_with_parents(dir, S_IRWXU) < -1) { > + fuse_err("%s: Failed to create directory %s: %s", > + __func__, dir, strerror(errno)); > + return -1; > + } > + > + socket_name = malloc(strlen(se->vu_socket_path) + 1); > + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); > + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); > + strreplace(socket_name, '/', '.'); Shall we use a local var to replace the long *se->vu_socket_path*? And I wonder if we could just place the lock file in the same dir level of socket file which could save some code work? > + > + se->vu_socket_lock_file = malloc(NAME_MAX); > + memset(se->vu_socket_lock_file, 0, NAME_MAX); > + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", > + dir, socket_name); > + > + if (!qemu_write_pidfile(se->vu_socket_lock_file, &local_err)) { > + error_report_err(local_err); Memory leak here. > + return -1; > + } > + > + free(socket_name); > + g_free(dir); > + > + return 0; > +} > + > static int fv_create_listen_socket(struct fuse_session *se) > { > struct sockaddr_un un; > @@ -876,6 +920,12 @@ static int fv_create_listen_socket(struct fuse_session *se) > return -1; > } > > + /* Check the vu_socket_path is already used */ > + if (fv_create_socket_lock_file(se) == -1) { > + fuse_err("%s: Socket lock file creation failed\n", __func__); > + return -1; > + } > + > /* Create the Unix socket to communicate with qemu > * based on QEMU's vhost-user-bridge > */ > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-12 3:12 ` piaojun @ 2019-08-12 19:02 ` Masayoshi Mizuma 2019-08-13 0:45 ` piaojun 0 siblings, 1 reply; 7+ messages in thread From: Masayoshi Mizuma @ 2019-08-12 19:02 UTC (permalink / raw) To: piaojun; +Cc: virtio-fs, Masayoshi Mizuma Hi Jun, On Mon, Aug 12, 2019 at 11:12:52AM +0800, piaojun wrote: > Hi Masayoshi, > > On 2019/8/12 6:55, Masayoshi Mizuma wrote: > > From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > > > > virtiofsd can run multiply even if the vhost_user_socket is > > same path. > > > > ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > > [1] 244965 > > virtio_session_mount: Waiting for vhost-user socket connection... > > ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > > [2] 244966 > > virtio_session_mount: Waiting for vhost-user socket connection... > > ]# > > > > The user will get confused about the situation and maybe the cause of the > > unexpected problem. So it's better to prevent the multiple running. > > > > Create a regular file under localstatedir directory to exclude the > > vhost_user_socket. To create and lock the file, use qemu_write_pidfile() > > because the API has some sanity checks and file lock. > > > > Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > > --- > > contrib/virtiofsd/fuse_i.h | 1 + > > contrib/virtiofsd/fuse_lowlevel.c | 3 ++ > > contrib/virtiofsd/fuse_virtio.c | 50 +++++++++++++++++++++++++++++++ > > 3 files changed, 54 insertions(+) > > > > diff --git a/contrib/virtiofsd/fuse_i.h b/contrib/virtiofsd/fuse_i.h > > index ff6e1b75ba..d31c675469 100644 > > --- a/contrib/virtiofsd/fuse_i.h > > +++ b/contrib/virtiofsd/fuse_i.h > > @@ -72,6 +72,7 @@ struct fuse_session { > > char *vu_socket_path; > > int vu_listen_fd; > > int vu_socketfd; > > + char *vu_socket_lock_file; > > I prefer char *vu_socket_lock which is just a personal habit. > > > struct fv_VuDev *virtio_dev; > > }; > > > > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c > > index 8adc4b1ab8..ab18b86435 100644 > > --- a/contrib/virtiofsd/fuse_lowlevel.c > > +++ b/contrib/virtiofsd/fuse_lowlevel.c > > @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) > > free(se->vu_socket_path); > > se->vu_socket_path = NULL; > > > > + unlink(se->vu_socket_lock_file); > > + free(se->vu_socket_lock_file); > > + > > free(se); > > } > > > > diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c > > index 083e4fc317..4999f95b63 100644 > > --- a/contrib/virtiofsd/fuse_virtio.c > > +++ b/contrib/virtiofsd/fuse_virtio.c > > @@ -32,6 +32,9 @@ > > > > #include "contrib/libvhost-user/libvhost-user.h" > > > > +#include "qemu/osdep.h" > > +#include "qapi/error.h" > > + > > struct fv_VuDev; > > struct fv_QueueInfo { > > pthread_t thread; > > @@ -862,6 +865,47 @@ int virtio_loop(struct fuse_session *se) > > return 0; > > } > > > > +static void strreplace(char *s, char old, char new) > > +{ > > + for (; *s; ++s) > > + if (*s == old) > > + *s = new; > > +} > > + > > +static int fv_create_socket_lock_file(struct fuse_session *se) > > +{ > > + char *dir, *socket_name; > > + Error *local_err = NULL; > > + > > + dir = qemu_get_local_state_pathname("run/virtiofsd"); > > + > > + if (g_mkdir_with_parents(dir, S_IRWXU) < -1) { > > + fuse_err("%s: Failed to create directory %s: %s", > > + __func__, dir, strerror(errno)); > > + return -1; > > + } > > + > > + socket_name = malloc(strlen(se->vu_socket_path) + 1); > > + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); > > + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); > > + strreplace(socket_name, '/', '.'); > > Shall we use a local var to replace the long *se->vu_socket_path*? And I'll add a local var to replace strlen(se->vu_socket_path). > I wonder if we could just place the lock file in the same dir level of > socket file which could save some code work? Thank you for the suggestion but I prefer the lock file is in localstatedir, for example /var/run/virtiofsd, because if the lock file in the same dir and the dir is used in common, for example /tmp, users may delete the lock file... > > > + > > + se->vu_socket_lock_file = malloc(NAME_MAX); > > + memset(se->vu_socket_lock_file, 0, NAME_MAX); > > + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", > > + dir, socket_name); > > + > > + if (!qemu_write_pidfile(se->vu_socket_lock_file, &local_err)) { > > + error_report_err(local_err); > > Memory leak here. I'll fix this. Thanks! Masa > > > + return -1; > > + } > > + > > + free(socket_name); > > + g_free(dir); > > + > > + return 0; > > +} > > + > > static int fv_create_listen_socket(struct fuse_session *se) > > { > > struct sockaddr_un un; > > @@ -876,6 +920,12 @@ static int fv_create_listen_socket(struct fuse_session *se) > > return -1; > > } > > > > + /* Check the vu_socket_path is already used */ > > + if (fv_create_socket_lock_file(se) == -1) { > > + fuse_err("%s: Socket lock file creation failed\n", __func__); > > + return -1; > > + } > > + > > /* Create the Unix socket to communicate with qemu > > * based on QEMU's vhost-user-bridge > > */ > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-12 19:02 ` Masayoshi Mizuma @ 2019-08-13 0:45 ` piaojun 2019-08-13 13:48 ` Masayoshi Mizuma 0 siblings, 1 reply; 7+ messages in thread From: piaojun @ 2019-08-13 0:45 UTC (permalink / raw) To: Masayoshi Mizuma; +Cc: virtio-fs, Masayoshi Mizuma On 2019/8/13 3:02, Masayoshi Mizuma wrote: > Hi Jun, > > On Mon, Aug 12, 2019 at 11:12:52AM +0800, piaojun wrote: >> Hi Masayoshi, >> >> On 2019/8/12 6:55, Masayoshi Mizuma wrote: >>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> >>> >>> virtiofsd can run multiply even if the vhost_user_socket is >>> same path. >>> >>> ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & >>> [1] 244965 >>> virtio_session_mount: Waiting for vhost-user socket connection... >>> ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & >>> [2] 244966 >>> virtio_session_mount: Waiting for vhost-user socket connection... >>> ]# >>> >>> The user will get confused about the situation and maybe the cause of the >>> unexpected problem. So it's better to prevent the multiple running. >>> >>> Create a regular file under localstatedir directory to exclude the >>> vhost_user_socket. To create and lock the file, use qemu_write_pidfile() >>> because the API has some sanity checks and file lock. >>> >>> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> >>> --- >>> contrib/virtiofsd/fuse_i.h | 1 + >>> contrib/virtiofsd/fuse_lowlevel.c | 3 ++ >>> contrib/virtiofsd/fuse_virtio.c | 50 +++++++++++++++++++++++++++++++ >>> 3 files changed, 54 insertions(+) >>> >>> diff --git a/contrib/virtiofsd/fuse_i.h b/contrib/virtiofsd/fuse_i.h >>> index ff6e1b75ba..d31c675469 100644 >>> --- a/contrib/virtiofsd/fuse_i.h >>> +++ b/contrib/virtiofsd/fuse_i.h >>> @@ -72,6 +72,7 @@ struct fuse_session { >>> char *vu_socket_path; >>> int vu_listen_fd; >>> int vu_socketfd; >>> + char *vu_socket_lock_file; >> >> I prefer char *vu_socket_lock which is just a personal habit. >> >>> struct fv_VuDev *virtio_dev; >>> }; >>> >>> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c >>> index 8adc4b1ab8..ab18b86435 100644 >>> --- a/contrib/virtiofsd/fuse_lowlevel.c >>> +++ b/contrib/virtiofsd/fuse_lowlevel.c >>> @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) >>> free(se->vu_socket_path); >>> se->vu_socket_path = NULL; >>> >>> + unlink(se->vu_socket_lock_file); >>> + free(se->vu_socket_lock_file); >>> + >>> free(se); >>> } >>> >>> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c >>> index 083e4fc317..4999f95b63 100644 >>> --- a/contrib/virtiofsd/fuse_virtio.c >>> +++ b/contrib/virtiofsd/fuse_virtio.c >>> @@ -32,6 +32,9 @@ >>> >>> #include "contrib/libvhost-user/libvhost-user.h" >>> >>> +#include "qemu/osdep.h" >>> +#include "qapi/error.h" >>> + >>> struct fv_VuDev; >>> struct fv_QueueInfo { >>> pthread_t thread; >>> @@ -862,6 +865,47 @@ int virtio_loop(struct fuse_session *se) >>> return 0; >>> } >>> >>> +static void strreplace(char *s, char old, char new) >>> +{ >>> + for (; *s; ++s) >>> + if (*s == old) >>> + *s = new; >>> +} >>> + >>> +static int fv_create_socket_lock_file(struct fuse_session *se) >>> +{ >>> + char *dir, *socket_name; >>> + Error *local_err = NULL; >>> + >>> + dir = qemu_get_local_state_pathname("run/virtiofsd"); >>> + >>> + if (g_mkdir_with_parents(dir, S_IRWXU) < -1) { >>> + fuse_err("%s: Failed to create directory %s: %s", >>> + __func__, dir, strerror(errno)); >>> + return -1; >>> + } >>> + >>> + socket_name = malloc(strlen(se->vu_socket_path) + 1); Sorry for not pointing this in the last email, and it's better adding a NULL checker here, moreover, checking NULL before *unlink* in fuse_session_destroy() becomes necessary. >>> + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); >>> + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); >>> + strreplace(socket_name, '/', '.'); >> >> Shall we use a local var to replace the long *se->vu_socket_path*? And > > I'll add a local var to replace strlen(se->vu_socket_path). > >> I wonder if we could just place the lock file in the same dir level of >> socket file which could save some code work? > > Thank you for the suggestion but I prefer the lock file is in > localstatedir, for example /var/run/virtiofsd, because if the > lock file in the same dir and the dir is used in common, for example > /tmp, users may delete the lock file... It's also good for me. And if you agree with all my comments, feel free to add: Reviewed-by: Jun Piao <piaojun@huawei.com> Jun > >> >>> + >>> + se->vu_socket_lock_file = malloc(NAME_MAX); >>> + memset(se->vu_socket_lock_file, 0, NAME_MAX); >>> + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", >>> + dir, socket_name); >>> + >>> + if (!qemu_write_pidfile(se->vu_socket_lock_file, &local_err)) { >>> + error_report_err(local_err); >> >> Memory leak here. > > I'll fix this. > > Thanks! > Masa > >> >>> + return -1; >>> + } >>> + >>> + free(socket_name); >>> + g_free(dir); >>> + >>> + return 0; >>> +} >>> + >>> static int fv_create_listen_socket(struct fuse_session *se) >>> { >>> struct sockaddr_un un; >>> @@ -876,6 +920,12 @@ static int fv_create_listen_socket(struct fuse_session *se) >>> return -1; >>> } >>> >>> + /* Check the vu_socket_path is already used */ >>> + if (fv_create_socket_lock_file(se) == -1) { >>> + fuse_err("%s: Socket lock file creation failed\n", __func__); >>> + return -1; >>> + } >>> + >>> /* Create the Unix socket to communicate with qemu >>> * based on QEMU's vhost-user-bridge >>> */ >>> > . > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-13 0:45 ` piaojun @ 2019-08-13 13:48 ` Masayoshi Mizuma 0 siblings, 0 replies; 7+ messages in thread From: Masayoshi Mizuma @ 2019-08-13 13:48 UTC (permalink / raw) To: piaojun; +Cc: virtio-fs, Masayoshi Mizuma On Tue, Aug 13, 2019 at 08:45:06AM +0800, piaojun wrote: > > > On 2019/8/13 3:02, Masayoshi Mizuma wrote: > > Hi Jun, > > > > On Mon, Aug 12, 2019 at 11:12:52AM +0800, piaojun wrote: > >> Hi Masayoshi, > >> > >> On 2019/8/12 6:55, Masayoshi Mizuma wrote: > >>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > >>> > >>> virtiofsd can run multiply even if the vhost_user_socket is > >>> same path. > >>> > >>> ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > >>> [1] 244965 > >>> virtio_session_mount: Waiting for vhost-user socket connection... > >>> ]# ./virtiofsd -o vhost_user_socket=/tmp/vhostqemu -o source=/tmp/share & > >>> [2] 244966 > >>> virtio_session_mount: Waiting for vhost-user socket connection... > >>> ]# > >>> > >>> The user will get confused about the situation and maybe the cause of the > >>> unexpected problem. So it's better to prevent the multiple running. > >>> > >>> Create a regular file under localstatedir directory to exclude the > >>> vhost_user_socket. To create and lock the file, use qemu_write_pidfile() > >>> because the API has some sanity checks and file lock. > >>> > >>> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com> > >>> --- > >>> contrib/virtiofsd/fuse_i.h | 1 + > >>> contrib/virtiofsd/fuse_lowlevel.c | 3 ++ > >>> contrib/virtiofsd/fuse_virtio.c | 50 +++++++++++++++++++++++++++++++ > >>> 3 files changed, 54 insertions(+) > >>> > >>> diff --git a/contrib/virtiofsd/fuse_i.h b/contrib/virtiofsd/fuse_i.h > >>> index ff6e1b75ba..d31c675469 100644 > >>> --- a/contrib/virtiofsd/fuse_i.h > >>> +++ b/contrib/virtiofsd/fuse_i.h > >>> @@ -72,6 +72,7 @@ struct fuse_session { > >>> char *vu_socket_path; > >>> int vu_listen_fd; > >>> int vu_socketfd; > >>> + char *vu_socket_lock_file; > >> > >> I prefer char *vu_socket_lock which is just a personal habit. > >> > >>> struct fv_VuDev *virtio_dev; > >>> }; > >>> > >>> diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c > >>> index 8adc4b1ab8..ab18b86435 100644 > >>> --- a/contrib/virtiofsd/fuse_lowlevel.c > >>> +++ b/contrib/virtiofsd/fuse_lowlevel.c > >>> @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) > >>> free(se->vu_socket_path); > >>> se->vu_socket_path = NULL; > >>> > >>> + unlink(se->vu_socket_lock_file); > >>> + free(se->vu_socket_lock_file); > >>> + > >>> free(se); > >>> } > >>> > >>> diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c > >>> index 083e4fc317..4999f95b63 100644 > >>> --- a/contrib/virtiofsd/fuse_virtio.c > >>> +++ b/contrib/virtiofsd/fuse_virtio.c > >>> @@ -32,6 +32,9 @@ > >>> > >>> #include "contrib/libvhost-user/libvhost-user.h" > >>> > >>> +#include "qemu/osdep.h" > >>> +#include "qapi/error.h" > >>> + > >>> struct fv_VuDev; > >>> struct fv_QueueInfo { > >>> pthread_t thread; > >>> @@ -862,6 +865,47 @@ int virtio_loop(struct fuse_session *se) > >>> return 0; > >>> } > >>> > >>> +static void strreplace(char *s, char old, char new) > >>> +{ > >>> + for (; *s; ++s) > >>> + if (*s == old) > >>> + *s = new; > >>> +} > >>> + > >>> +static int fv_create_socket_lock_file(struct fuse_session *se) > >>> +{ > >>> + char *dir, *socket_name; > >>> + Error *local_err = NULL; > >>> + > >>> + dir = qemu_get_local_state_pathname("run/virtiofsd"); > >>> + > >>> + if (g_mkdir_with_parents(dir, S_IRWXU) < -1) { > >>> + fuse_err("%s: Failed to create directory %s: %s", > >>> + __func__, dir, strerror(errno)); > >>> + return -1; > >>> + } > >>> + > >>> + socket_name = malloc(strlen(se->vu_socket_path) + 1); > > Sorry for not pointing this in the last email, and it's better adding a > NULL checker here, moreover, checking NULL before *unlink* in > fuse_session_destroy() becomes necessary. Good point, thanks. > > >>> + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); > >>> + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); > >>> + strreplace(socket_name, '/', '.'); > >> > >> Shall we use a local var to replace the long *se->vu_socket_path*? And > > > > I'll add a local var to replace strlen(se->vu_socket_path). > > > >> I wonder if we could just place the lock file in the same dir level of > >> socket file which could save some code work? > > > > Thank you for the suggestion but I prefer the lock file is in > > localstatedir, for example /var/run/virtiofsd, because if the > > lock file in the same dir and the dir is used in common, for example > > /tmp, users may delete the lock file... > > It's also good for me. And if you agree with all my comments, feel free > to add: > > Reviewed-by: Jun Piao <piaojun@huawei.com> Thanks! - Masa > > Jun > > > > >> > >>> + > >>> + se->vu_socket_lock_file = malloc(NAME_MAX); > >>> + memset(se->vu_socket_lock_file, 0, NAME_MAX); > >>> + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", > >>> + dir, socket_name); > >>> + > >>> + if (!qemu_write_pidfile(se->vu_socket_lock_file, &local_err)) { > >>> + error_report_err(local_err); > >> > >> Memory leak here. > > > > I'll fix this. > > > > Thanks! > > Masa > > > >> > >>> + return -1; > >>> + } > >>> + > >>> + free(socket_name); > >>> + g_free(dir); > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int fv_create_listen_socket(struct fuse_session *se) > >>> { > >>> struct sockaddr_un un; > >>> @@ -876,6 +920,12 @@ static int fv_create_listen_socket(struct fuse_session *se) > >>> return -1; > >>> } > >>> > >>> + /* Check the vu_socket_path is already used */ > >>> + if (fv_create_socket_lock_file(se) == -1) { > >>> + fuse_err("%s: Socket lock file creation failed\n", __func__); > >>> + return -1; > >>> + } > >>> + > >>> /* Create the Unix socket to communicate with qemu > >>> * based on QEMU's vhost-user-bridge > >>> */ > >>> > > . > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-11 22:55 [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket Masayoshi Mizuma 2019-08-12 3:12 ` piaojun @ 2019-08-13 15:51 ` Stefan Hajnoczi 2019-08-13 18:33 ` Masayoshi Mizuma 1 sibling, 1 reply; 7+ messages in thread From: Stefan Hajnoczi @ 2019-08-13 15:51 UTC (permalink / raw) To: Masayoshi Mizuma; +Cc: virtio-fs, Masayoshi Mizuma [-- Attachment #1: Type: text/plain, Size: 1486 bytes --] On Sun, Aug 11, 2019 at 06:55:34PM -0400, Masayoshi Mizuma wrote: > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c > index 8adc4b1ab8..ab18b86435 100644 > --- a/contrib/virtiofsd/fuse_lowlevel.c > +++ b/contrib/virtiofsd/fuse_lowlevel.c > @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) > free(se->vu_socket_path); > se->vu_socket_path = NULL; > > + unlink(se->vu_socket_lock_file); Are you sure this works? We should be inside the chroot here, so this is probably not the same file that we created! > + socket_name = malloc(strlen(se->vu_socket_path) + 1); > + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); > + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); These lines can be replaced with: socket_name = g_strdup(se->vu_socket_path); ... g_free(socket_name); > + strreplace(socket_name, '/', '.'); > + > + se->vu_socket_lock_file = malloc(NAME_MAX); > + memset(se->vu_socket_lock_file, 0, NAME_MAX); > + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", > + dir, socket_name); These lines can be replaced with: se->vu_socket_lock_file = g_strdup_printf("%s/%s.pid", dir, socket_name); The difference here is that it won't silently truncate to NAME_MAX. This is probably a good thing since an ENAMETOOLONG error should be reported instead of silently truncating the path. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket 2019-08-13 15:51 ` Stefan Hajnoczi @ 2019-08-13 18:33 ` Masayoshi Mizuma 0 siblings, 0 replies; 7+ messages in thread From: Masayoshi Mizuma @ 2019-08-13 18:33 UTC (permalink / raw) To: Stefan Hajnoczi; +Cc: virtio-fs, Masayoshi Mizuma On Tue, Aug 13, 2019 at 04:51:53PM +0100, Stefan Hajnoczi wrote: > On Sun, Aug 11, 2019 at 06:55:34PM -0400, Masayoshi Mizuma wrote: > > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c > > index 8adc4b1ab8..ab18b86435 100644 > > --- a/contrib/virtiofsd/fuse_lowlevel.c > > +++ b/contrib/virtiofsd/fuse_lowlevel.c > > @@ -2587,6 +2587,9 @@ void fuse_session_destroy(struct fuse_session *se) > > free(se->vu_socket_path); > > se->vu_socket_path = NULL; > > > > + unlink(se->vu_socket_lock_file); > > Are you sure this works? We should be inside the chroot here, so this > is probably not the same file that we created! Ah, thank you for pointing it out. You're right. The namespace is changed into the sandbox after the vhost-user socket connection is established. I'll remove the unlink(). > > > + socket_name = malloc(strlen(se->vu_socket_path) + 1); > > + memset(socket_name, 0, strlen(se->vu_socket_path) + 1); > > + memcpy(socket_name, se->vu_socket_path, strlen(se->vu_socket_path)); > > These lines can be replaced with: > > socket_name = g_strdup(se->vu_socket_path); > ... > g_free(socket_name); Thanks. > > > + strreplace(socket_name, '/', '.'); > > + > > + se->vu_socket_lock_file = malloc(NAME_MAX); > > + memset(se->vu_socket_lock_file, 0, NAME_MAX); > > + snprintf(se->vu_socket_lock_file, NAME_MAX, "%s/%s.pid", > > + dir, socket_name); > > These lines can be replaced with: > > se->vu_socket_lock_file = g_strdup_printf("%s/%s.pid", dir, socket_name); > > The difference here is that it won't silently truncate to NAME_MAX. > This is probably a good thing since an ENAMETOOLONG error should be > reported instead of silently truncating the path. Got it. I'll fix this. Thanks! Masa ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-13 18:33 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-11 22:55 [Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket Masayoshi Mizuma 2019-08-12 3:12 ` piaojun 2019-08-12 19:02 ` Masayoshi Mizuma 2019-08-13 0:45 ` piaojun 2019-08-13 13:48 ` Masayoshi Mizuma 2019-08-13 15:51 ` Stefan Hajnoczi 2019-08-13 18:33 ` Masayoshi Mizuma
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.