[Qemu-devel] [PULL v2 0/4] 9pfs/virtfs patches for 2.11 20170905

[Qemu-devel] [PULL v2 0/4] 9pfs/virtfs patches for 2.11 20170905

[Greg Kurz]

The following changes since commit 53e2c48d3f0db6a1598f49baf0b56dd4975e53a7:

  Merge remote-tracking branch 'remotes/mcayland/tags/qemu-sparc-signed' into staging (2017-09-04 18:53:46 +0100)

are available in the git repository at:

  https://github.com/gkurz/qemu.git tags/for-upstream

for you to fetch changes up to 32b6943699948f7adc35ada233fbd25daffad5e9:

  virtfs: error out gracefully when mandatory suboptions are missing (2017-09-05 17:56:58 +0200)

----------------------------------------------------------------
Some trivial fixes/cleanup and a fix to cause QEMU to error out gracefully
instead of aborting.

----------------------------------------------------------------
Greg Kurz (2):
      9pfs: local: clarify fchmodat_nofollow() implementation
      virtfs: error out gracefully when mandatory suboptions are missing

Philippe Mathieu-Daudé (1):
      9pfs: avoid sign conversion error simplifying the code

ZhiPeng Lu (1):
      fsdev: fix memory leak in main()

 fsdev/virtfs-proxy-helper.c |  2 ++
 hw/9pfs/9p-local.c          | 12 ++++++++----
 hw/9pfs/9p.c                |  6 ++----
 vl.c                        | 16 ++++++++++------
 4 files changed, 22 insertions(+), 14 deletions(-)
-- 
2.13.5

[Qemu-devel] [PULL v2 1/4] 9pfs: avoid sign conversion error simplifying the code

[Greg Kurz]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

(note this is how other functions also handle the errors).

hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion
        offset = err;
                 ^~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/9pfs/9p.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index 333dbb6f8ee2..0a37c8bd1361 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque)
     v9fs_string_init(&version);
     err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
     trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data);
@@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque)
 
     err = pdu_marshal(pdu, offset, "ds", s->msize, &version);
     if (err < 0) {
-        offset = err;
         goto out;
     }
-    offset += err;
+    err += offset;
     trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data);
 out:
-    pdu_complete(pdu, offset);
+    pdu_complete(pdu, err);
     v9fs_string_free(&version);
 }
 
-- 
2.13.5

[Qemu-devel] [PULL v2 2/4] fsdev: fix memory leak in main()

[Greg Kurz]

From: ZhiPeng Lu <lu.zhipeng@zte.com.cn>

@rpath and @sock_name are not freed and leaked.

[groug, not really leaked since the program exits just after that. But it
 is always good practice to free allocated memory]

Signed-off-by: Zhipeng Lu <lu.zhipeng@zte.com.cn>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 fsdev/virtfs-proxy-helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index 6c066ec9a0ce..8e48500dd53a 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -1162,6 +1162,8 @@ int main(int argc, char **argv)
 
     process_requests(sock);
 error:
+    g_free(rpath);
+    g_free(sock_name);
     do_log(LOG_INFO, "Done\n");
     closelog();
     return 0;
-- 
2.13.5

[Qemu-devel] [PULL v2 3/4] 9pfs: local: clarify fchmodat_nofollow() implementation

[Greg Kurz]

Since fchmodat(2) on Linux doesn't support AT_SYMLINK_NOFOLLOW, we have to
implement it using workarounds. There are two different ways, depending on
whether the system supports O_PATH or not.

In the case O_PATH is supported, we rely on the behavhior of openat(2)
when passing O_NOFOLLOW | O_PATH and the file is a symbolic link. Even
if openat_file() already adds O_NOFOLLOW to the flags, this patch makes
it explicit that we need both creation flags to obtain the expected
behavior.

This is only cleanup, no functional change.

Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/9pfs/9p-local.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index efb0b79a74bf..e51af87309c6 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -349,11 +349,11 @@ static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode)
         return -1;
     }
 
-    /* Access modes are ignored when O_PATH is supported. We try O_RDONLY and
-     * O_WRONLY for old-systems that don't support O_PATH.
-     */
-    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0);
+    fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0);
 #if O_PATH_9P_UTIL == 0
+    /* Fallback for systems that don't support O_PATH: we depend on the file
+     * being readable or writable.
+     */
     if (fd == -1) {
         /* In case the file is writable-only and isn't a directory. */
         if (errno == EACCES) {
@@ -368,6 +368,10 @@ static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode)
     }
     ret = fchmod(fd, mode);
 #else
+    /* Access modes are ignored when O_PATH is supported. If name is a symbolic
+     * link, O_PATH | O_NOFOLLOW causes openat(2) to return a file descriptor
+     * referring to the symbolic link.
+     */
     if (fd == -1) {
         return -1;
     }
-- 
2.13.5

[Qemu-devel] [PULL v2 4/4] virtfs: error out gracefully when mandatory suboptions are missing

[Greg Kurz]

We internally convert -virtfs to -fsdev/-device. If the user doesn't
provide the path or security_model suboptions, and the fsdev backend
requires them, we hit an assertion when populating the internal -fsdev
option:

util/qemu-option.c:547: opt_set: Assertion `opt->str' failed.
Aborted (core dumped)

Let's test the suboption presence on the command line before trying
to set it in the internal -fsdev option, and let the backend code
error out gracefully (ie, like it already does when the user passes
-fsdev on the command line).

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 vl.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/vl.c b/vl.c
index 0b45e1b6fa31..e75757f9772e 100644
--- a/vl.c
+++ b/vl.c
@@ -3557,7 +3557,7 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_virtfs: {
                 QemuOpts *fsdev;
                 QemuOpts *device;
-                const char *writeout, *sock_fd, *socket;
+                const char *writeout, *sock_fd, *socket, *path, *security_model;
 
                 olist = qemu_find_opts("virtfs");
                 if (!olist) {
@@ -3596,11 +3596,15 @@ int main(int argc, char **argv, char **envp)
                 }
                 qemu_opt_set(fsdev, "fsdriver",
                              qemu_opt_get(opts, "fsdriver"), &error_abort);
-                qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"),
-                             &error_abort);
-                qemu_opt_set(fsdev, "security_model",
-                             qemu_opt_get(opts, "security_model"),
-                             &error_abort);
+                path = qemu_opt_get(opts, "path");
+                if (path) {
+                    qemu_opt_set(fsdev, "path", path, &error_abort);
+                }
+                security_model = qemu_opt_get(opts, "security_model");
+                if (security_model) {
+                    qemu_opt_set(fsdev, "security_model", security_model,
+                                 &error_abort);
+                }
                 socket = qemu_opt_get(opts, "socket");
                 if (socket) {
                     qemu_opt_set(fsdev, "socket", socket, &error_abort);
-- 
2.13.5

Re: [Qemu-devel] [PULL v2 0/4] 9pfs/virtfs patches for 2.11 20170905

[Peter Maydell]

On 5 September 2017 at 17:00, Greg Kurz <groug@kaod.org> wrote:
> The following changes since commit 53e2c48d3f0db6a1598f49baf0b56dd4975e53a7:
>
>   Merge remote-tracking branch 'remotes/mcayland/tags/qemu-sparc-signed' into staging (2017-09-04 18:53:46 +0100)
>
> are available in the git repository at:
>
>   https://github.com/gkurz/qemu.git tags/for-upstream
>
> for you to fetch changes up to 32b6943699948f7adc35ada233fbd25daffad5e9:
>
>   virtfs: error out gracefully when mandatory suboptions are missing (2017-09-05 17:56:58 +0200)
>
> ----------------------------------------------------------------
> Some trivial fixes/cleanup and a fix to cause QEMU to error out gracefully
> instead of aborting.
>
> ----------------------------------------------------------------
> Greg Kurz (2):
>       9pfs: local: clarify fchmodat_nofollow() implementation
>       virtfs: error out gracefully when mandatory suboptions are missing
>
> Philippe Mathieu-Daudé (1):
>       9pfs: avoid sign conversion error simplifying the code
>
> ZhiPeng Lu (1):
>       fsdev: fix memory leak in main()

Applied, thanks.

-- PMM