* [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.