[Virtio-fs] [PATCH v2] virtiofsd: Prevent multiply running with same vhost_user_socket
Masayoshi Mizuma
msys.mizuma at gmail.com
Tue Aug 13 13:48:09 UTC 2019
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 at 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 at 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 at 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
> >>> */
> >>>
> > .
> >
More information about the Virtio-fs
mailing list