[Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Corey Bryant]

libvirt's sVirt security driver provides SELinux MAC isolation for
Qemu guest processes and their corresponding image files.  In other
words, sVirt uses SELinux to prevent a QEMU process from opening
files that do not belong to it.

sVirt provides this support by labeling guests and resources with
security labels that are stored in file system extended attributes.
Some file systems, such as NFS, do not support the extended
attribute security namespace, and therefore cannot support sVirt
isolation.

A solution to this problem is to provide fd passing support, where
libvirt opens files and passes file descriptors to QEMU.  This,
along with SELinux policy to prevent QEMU from opening files, can
provide image file isolation for NFS files stored on the same NFS
mount.

This patch series adds the pass-fd QMP monitor command, which allows
an fd to be passed via SCM_RIGHTS, and returns the received file
descriptor.  Support is also added to the block layer to allow QEMU
to dup the fd when the filename is of the /dev/fd/X format.  This
is useful if MAC policy prevents QEMU from opening specific types
of files.

One nice thing about this approach is that no new SELinux policy is
required to prevent open of NFS files (files with type nfs_t).  The
virt_use_nfs boolean type simply needs to be set to false, and open
will be prevented (and dup will be allowed).  For example:

    # setsebool virt_use_nfs 0
    # getsebool virt_use_nfs
    virt_use_nfs --> off

Corey Bryant (5):
  qapi: Convert getfd and closefd
  qapi: Add pass-fd QMP command
  osdep: Enable qemu_open to dup pre-opened fd
  block: Convert open calls to qemu_open
  block: Prevent /dev/fd/X filename from being detected as floppy

 block/raw-posix.c |   22 ++++++++++---------
 block/raw-win32.c |    4 ++--
 block/vdi.c       |    5 +++--
 block/vmdk.c      |   21 ++++++++----------
 block/vpc.c       |    2 +-
 block/vvfat.c     |   21 +++++++++---------
 hmp-commands.hx   |    6 ++----
 hmp.c             |   18 ++++++++++++++++
 hmp.h             |    2 ++
 monitor.c         |   61 +++++++++++++++++++++++++++++++++++++++--------------
 osdep.c           |   13 ++++++++++++
 qapi-schema.json  |   54 +++++++++++++++++++++++++++++++++++++++++++++++
 qmp-commands.hx   |   48 +++++++++++++++++++++++++++++++++++++----
 13 files changed, 216 insertions(+), 61 deletions(-)

-- 
1.7.10.2

[Qemu-devel] [PATCH v3 1/5] qapi: Convert getfd and closefd

[Corey Bryant]

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2:
 -Convert getfd and closefd to QAPI (lcapitulino@redhat.com)
 -Remove changes that returned fd from getfd (lcapitulino@redhat.com)
 -Wrap hmp_* functions around qmp_* functions (kwolf@redhat.com)
 -Move hmp_* functions to hmp.c (lcapitulino@redhat.com)
 -Drop .user_print lines (lcapitulino@redhat.com)
 -Use 'cmd' instead of 'cmd_new' for HMP (lcapitulino@redhat.com)
 -Change QMP command existance back to 0.14 (lcapitulino@redhat.com)

v3:
 -Remove unnecessary return statements from qmp_* functions
  (lcapitulino@redhat.com)
 -Add notes to QMP command describing behavior in more detail
  (lcapitulino@redhat.com, eblake@redhat.com)

 hmp-commands.hx  |    6 ++----
 hmp.c            |   18 ++++++++++++++++++
 hmp.h            |    2 ++
 monitor.c        |   32 ++++++++++++++------------------
 qapi-schema.json |   35 +++++++++++++++++++++++++++++++++++
 qmp-commands.hx  |   14 ++++++++++----
 6 files changed, 81 insertions(+), 26 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index f5d9d91..eea8b32 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1236,8 +1236,7 @@ ETEXI
         .args_type  = "fdname:s",
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_getfd,
+        .mhandler.cmd = hmp_getfd,
     },
 
 STEXI
@@ -1253,8 +1252,7 @@ ETEXI
         .args_type  = "fdname:s",
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_closefd,
+        .mhandler.cmd = hmp_closefd,
     },
 
 STEXI
diff --git a/hmp.c b/hmp.c
index 2ce8cb9..6241856 100644
--- a/hmp.c
+++ b/hmp.c
@@ -999,3 +999,21 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     qmp_netdev_del(id, &err);
     hmp_handle_error(mon, &err);
 }
+
+void hmp_getfd(Monitor *mon, const QDict *qdict)
+{
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    Error *errp = NULL;
+
+    qmp_getfd(fdname, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
+void hmp_closefd(Monitor *mon, const QDict *qdict)
+{
+    const char *fdname = qdict_get_str(qdict, "fdname");
+    Error *errp = NULL;
+
+    qmp_closefd(fdname, &errp);
+    hmp_handle_error(mon, &errp);
+}
diff --git a/hmp.h b/hmp.h
index 79d138d..8d2b0d7 100644
--- a/hmp.h
+++ b/hmp.h
@@ -64,5 +64,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict);
 void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict);
 void hmp_netdev_add(Monitor *mon, const QDict *qdict);
 void hmp_netdev_del(Monitor *mon, const QDict *qdict);
+void hmp_getfd(Monitor *mon, const QDict *qdict);
+void hmp_closefd(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/monitor.c b/monitor.c
index a3bc2c7..1a7f7e7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2182,48 +2182,45 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
 }
 #endif
 
-static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_getfd(const char *fdname, Error **errp)
 {
-    const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
     int fd;
 
-    fd = qemu_chr_fe_get_msgfd(mon->chr);
+    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
     if (fd == -1) {
-        qerror_report(QERR_FD_NOT_SUPPLIED);
-        return -1;
+        error_set(errp, QERR_FD_NOT_SUPPLIED);
+        return;
     }
 
     if (qemu_isdigit(fdname[0])) {
-        qerror_report(QERR_INVALID_PARAMETER_VALUE, "fdname",
-                      "a name not starting with a digit");
-        return -1;
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                  "a name not starting with a digit");
+        return;
     }
 
-    QLIST_FOREACH(monfd, &mon->fds, next) {
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
 
         close(monfd->fd);
         monfd->fd = fd;
-        return 0;
+        return;
     }
 
     monfd = g_malloc0(sizeof(mon_fd_t));
     monfd->name = g_strdup(fdname);
     monfd->fd = fd;
 
-    QLIST_INSERT_HEAD(&mon->fds, monfd, next);
-    return 0;
+    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
 }
 
-static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
+void qmp_closefd(const char *fdname, Error **errp)
 {
-    const char *fdname = qdict_get_str(qdict, "fdname");
     mon_fd_t *monfd;
 
-    QLIST_FOREACH(monfd, &mon->fds, next) {
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
         if (strcmp(monfd->name, fdname) != 0) {
             continue;
         }
@@ -2232,11 +2229,10 @@ static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data)
         close(monfd->fd);
         g_free(monfd->name);
         g_free(monfd);
-        return 0;
+        return;
     }
 
-    qerror_report(QERR_FD_NOT_FOUND, fdname);
-    return -1;
+    error_set(errp, QERR_FD_NOT_FOUND, fdname);
 }
 
 static void do_loadvm(Monitor *mon, const QDict *qdict)
diff --git a/qapi-schema.json b/qapi-schema.json
index 3b6e346..26a6b84 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1862,3 +1862,38 @@
 # Since: 0.14.0
 ##
 { 'command': 'netdev_del', 'data': {'id': 'str'} }
+
+##
+# @getfd:
+#
+# Receive a file descriptor via SCM rights and assign it a name
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdname is not valid, InvalidParameterType
+#
+# Since: 0.14.0
+#
+# Notes: If @fdname already exists, the file descriptor assigned to
+#        it will be closed and replaced by the received file
+#        descriptor.
+#        The 'closefd' command can be used to explicitly close the
+#        file descriptor when it is no longer needed.
+##
+{ 'command': 'getfd', 'data': {'fdname': 'str'} }
+
+##
+# @closefd:
+#
+# Close a file descriptor previously passed via SCM rights
+#
+# @fdname: file descriptor name
+#
+# Returns: Nothing on success
+#          If @fdname is not found, FdNotFound
+#
+# Since: 0.14.0
+##
+{ 'command': 'closefd', 'data': {'fdname': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 2e1a38e..e3cf3c5 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -873,8 +873,7 @@ EQMP
         .args_type  = "fdname:s",
         .params     = "getfd name",
         .help       = "receive a file descriptor via SCM rights and assign it a name",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_getfd,
+        .mhandler.cmd_new = qmp_marshal_input_getfd,
     },
 
 SQMP
@@ -892,6 +891,14 @@ Example:
 -> { "execute": "getfd", "arguments": { "fdname": "fd1" } }
 <- { "return": {} }
 
+Notes:
+
+(1) If the name specified by the "fdname" argument already exists,
+    the file descriptor assigned to it will be closed and replaced
+    by the received file descriptor.
+(2) The 'closefd' command can be used to explicitly close the file
+    descriptor when it is no longer needed.
+
 EQMP
 
     {
@@ -899,8 +906,7 @@ EQMP
         .args_type  = "fdname:s",
         .params     = "closefd name",
         .help       = "close a file descriptor previously passed via SCM rights",
-        .user_print = monitor_user_noop,
-        .mhandler.cmd_new = do_closefd,
+        .mhandler.cmd_new = qmp_marshal_input_closefd,
     },
 
 SQMP
-- 
1.7.10.2

[Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Corey Bryant]

This patch adds the pass-fd QMP command using the QAPI framework.
Like the getfd command, it is used to pass a file descriptor via
SCM_RIGHTS.  However, the pass-fd command also returns the received
file descriptor, which is a difference in behavior from the getfd
command, which returns nothing.

The closefd command can be used to close a file descriptor that was
passed with the pass-fd command.

Note that when using getfd or pass-fd, there are some commands
(e.g. migrate with fd:name) that implicitly close the named fd.
When this is not the case, closefd must be used to avoid fd leaks.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2:
 -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
 -Use passfd as command name (berrange@redhat.com)

v3:
 -Use pass-fd as command name (lcapitulino@redhat.com)
 -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
 -Add notes to QMP command describing behavior in more detail
  (lcapitulino@redhat.com, eblake@redhat.com)
 -Add note about fd leakage (eblake@redhat.com)

 monitor.c        |   33 +++++++++++++++++++++++++++++++++
 qapi-schema.json |   19 +++++++++++++++++++
 qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+)

diff --git a/monitor.c b/monitor.c
index 1a7f7e7..6d99368 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
 }
 #endif
 
+int64_t qmp_pass_fd(const char *fdname, Error **errp)
+{
+    mon_fd_t *monfd;
+    int fd;
+
+    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
+    if (fd == -1) {
+        error_set(errp, QERR_FD_NOT_SUPPLIED);
+        return -1;
+    }
+
+    if (qemu_isdigit(fdname[0])) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                  "a name not starting with a digit");
+        return -1;
+    }
+
+    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
+        if (strcmp(monfd->name, fdname) == 0) {
+            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
+                      "a name that does not already exist");
+            return -1;
+        }
+    }
+
+    monfd = g_malloc0(sizeof(mon_fd_t));
+    monfd->name = g_strdup(fdname);
+    monfd->fd = fd;
+
+    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
+    return fd;
+}
+
 void qmp_getfd(const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
diff --git a/qapi-schema.json b/qapi-schema.json
index 26a6b84..ed99f23 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1864,6 +1864,25 @@
 { 'command': 'netdev_del', 'data': {'id': 'str'} }
 
 ##
+# @pass-fd:
+#
+# Pass a file descriptor via SCM rights and assign it a name
+#
+# @fdname: file descriptor name
+#
+# Returns: The QEMU file descriptor that was received
+#          If file descriptor was not received, FdNotSupplied
+#          If @fdname is not valid, InvalidParameterType
+#
+# Since: 1.2.0
+#
+# Notes: If @fdname already exists, the command will fail.
+#        The 'closefd' command can be used to explicitly close the
+#        file descriptor when it is no longer needed.
+##
+{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
+
+##
 # @getfd:
 #
 # Receive a file descriptor via SCM rights and assign it a name
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e3cf3c5..c039947 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -869,6 +869,40 @@ Example:
 EQMP
 
     {
+        .name       = "pass-fd",
+        .args_type  = "fdname:s",
+        .params     = "pass-fd name",
+        .help       = "pass a file descriptor via SCM rights and assign it a name",
+        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
+    },
+
+SQMP
+pass-fd
+-------
+
+Pass a file descriptor via SCM rights and assign it a name.
+
+Arguments:
+
+- "fdname": file descriptor name (json-string)
+
+Return a json-int with the QEMU file descriptor that was received.
+
+Example:
+
+-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
+<- { "return": 42 }
+
+Notes:
+
+(1) If the name specified by the "fdname" argument already exists,
+    the command will fail.
+(2) The 'closefd' command can be used to explicitly close the file
+    descriptor when it is no longer needed.
+
+EQMP
+
+    {
         .name       = "getfd",
         .args_type  = "fdname:s",
         .params     = "getfd name",
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Luiz Capitulino]

On Thu, 14 Jun 2012 11:55:02 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> This patch adds the pass-fd QMP command using the QAPI framework.
> Like the getfd command, it is used to pass a file descriptor via
> SCM_RIGHTS.  However, the pass-fd command also returns the received
> file descriptor, which is a difference in behavior from the getfd
> command, which returns nothing.
> 
> The closefd command can be used to close a file descriptor that was
> passed with the pass-fd command.
> 
> Note that when using getfd or pass-fd, there are some commands
> (e.g. migrate with fd:name) that implicitly close the named fd.
> When this is not the case, closefd must be used to avoid fd leaks.
> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
> v2:
>  -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
>  -Use passfd as command name (berrange@redhat.com)
> 
> v3:
>  -Use pass-fd as command name (lcapitulino@redhat.com)
>  -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
>  -Add notes to QMP command describing behavior in more detail
>   (lcapitulino@redhat.com, eblake@redhat.com)
>  -Add note about fd leakage (eblake@redhat.com)
> 
>  monitor.c        |   33 +++++++++++++++++++++++++++++++++
>  qapi-schema.json |   19 +++++++++++++++++++
>  qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 1a7f7e7..6d99368 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>  }
>  #endif
>  
> +int64_t qmp_pass_fd(const char *fdname, Error **errp)
> +{
> +    mon_fd_t *monfd;
> +    int fd;
> +
> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
> +    if (fd == -1) {
> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> +        return -1;
> +    }
> +
> +    if (qemu_isdigit(fdname[0])) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> +                  "a name not starting with a digit");
> +        return -1;
> +    }
> +
> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> +        if (strcmp(monfd->name, fdname) == 0) {
> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> +                      "a name that does not already exist");
> +            return -1;
> +        }
> +    }

Returning the same error class for two different errors is not a good idea.
I think you have two options here. You could return QERR_INVALID_PARAMETER
for the "already exists" case or introduce QERR_FD_EXISTS. The later is
certainly nicer, but we were trying to avoid having too specific errors...

> +
> +    monfd = g_malloc0(sizeof(mon_fd_t));
> +    monfd->name = g_strdup(fdname);
> +    monfd->fd = fd;

Maybe you could try to move this to a separate function to share code with
qmp_getfd()?

> +
> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> +    return fd;
> +}
> +
>  void qmp_getfd(const char *fdname, Error **errp)
>  {
>      mon_fd_t *monfd;
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 26a6b84..ed99f23 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1864,6 +1864,25 @@
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @pass-fd:
> +#
> +# Pass a file descriptor via SCM rights and assign it a name
> +#
> +# @fdname: file descriptor name
> +#
> +# Returns: The QEMU file descriptor that was received
> +#          If file descriptor was not received, FdNotSupplied
> +#          If @fdname is not valid, InvalidParameterType
> +#
> +# Since: 1.2.0
> +#
> +# Notes: If @fdname already exists, the command will fail.
> +#        The 'closefd' command can be used to explicitly close the
> +#        file descriptor when it is no longer needed.
> +##
> +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
> +
> +##
>  # @getfd:
>  #
>  # Receive a file descriptor via SCM rights and assign it a name
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index e3cf3c5..c039947 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -869,6 +869,40 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "pass-fd",
> +        .args_type  = "fdname:s",
> +        .params     = "pass-fd name",
> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
> +    },
> +
> +SQMP
> +pass-fd
> +-------
> +
> +Pass a file descriptor via SCM rights and assign it a name.
> +
> +Arguments:
> +
> +- "fdname": file descriptor name (json-string)
> +
> +Return a json-int with the QEMU file descriptor that was received.
> +
> +Example:
> +
> +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
> +<- { "return": 42 }
> +
> +Notes:
> +
> +(1) If the name specified by the "fdname" argument already exists,
> +    the command will fail.
> +(2) The 'closefd' command can be used to explicitly close the file
> +    descriptor when it is no longer needed.
> +
> +EQMP
> +
> +    {
>          .name       = "getfd",
>          .args_type  = "fdname:s",
>          .params     = "getfd name",

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Corey Bryant]

On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
> On Thu, 14 Jun 2012 11:55:02 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> This patch adds the pass-fd QMP command using the QAPI framework.
>> Like the getfd command, it is used to pass a file descriptor via
>> SCM_RIGHTS.  However, the pass-fd command also returns the received
>> file descriptor, which is a difference in behavior from the getfd
>> command, which returns nothing.
>>
>> The closefd command can be used to close a file descriptor that was
>> passed with the pass-fd command.
>>
>> Note that when using getfd or pass-fd, there are some commands
>> (e.g. migrate with fd:name) that implicitly close the named fd.
>> When this is not the case, closefd must be used to avoid fd leaks.
>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>> v2:
>>   -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
>>   -Use passfd as command name (berrange@redhat.com)
>>
>> v3:
>>   -Use pass-fd as command name (lcapitulino@redhat.com)
>>   -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
>>   -Add notes to QMP command describing behavior in more detail
>>    (lcapitulino@redhat.com, eblake@redhat.com)
>>   -Add note about fd leakage (eblake@redhat.com)
>>
>>   monitor.c        |   33 +++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   19 +++++++++++++++++++
>>   qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 86 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 1a7f7e7..6d99368 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>>   }
>>   #endif
>>
>> +int64_t qmp_pass_fd(const char *fdname, Error **errp)
>> +{
>> +    mon_fd_t *monfd;
>> +    int fd;
>> +
>> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>> +    if (fd == -1) {
>> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
>> +        return -1;
>> +    }
>> +
>> +    if (qemu_isdigit(fdname[0])) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>> +                  "a name not starting with a digit");
>> +        return -1;
>> +    }
>> +
>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>> +        if (strcmp(monfd->name, fdname) == 0) {
>> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>> +                      "a name that does not already exist");
>> +            return -1;
>> +        }
>> +    }
>
> Returning the same error class for two different errors is not a good idea.
> I think you have two options here. You could return QERR_INVALID_PARAMETER
> for the "already exists" case or introduce QERR_FD_EXISTS. The later is
> certainly nicer, but we were trying to avoid having too specific errors...
>

I'm not clear on what the problem is with returning the same error class 
for two different errors.  Could you explain?  I don't have a problem 
changing it if it's an issue.

>> +
>> +    monfd = g_malloc0(sizeof(mon_fd_t));
>> +    monfd->name = g_strdup(fdname);
>> +    monfd->fd = fd;
>
> Maybe you could try to move this to a separate function to share code with
> qmp_getfd()?
>

Sure, no problem.  I can do that.

>> +
>> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>> +    return fd;
>> +}
>> +
>>   void qmp_getfd(const char *fdname, Error **errp)
>>   {
>>       mon_fd_t *monfd;
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 26a6b84..ed99f23 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1864,6 +1864,25 @@
>>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
>>
>>   ##
>> +# @pass-fd:
>> +#
>> +# Pass a file descriptor via SCM rights and assign it a name
>> +#
>> +# @fdname: file descriptor name
>> +#
>> +# Returns: The QEMU file descriptor that was received
>> +#          If file descriptor was not received, FdNotSupplied
>> +#          If @fdname is not valid, InvalidParameterType
>> +#
>> +# Since: 1.2.0
>> +#
>> +# Notes: If @fdname already exists, the command will fail.
>> +#        The 'closefd' command can be used to explicitly close the
>> +#        file descriptor when it is no longer needed.
>> +##
>> +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
>> +
>> +##
>>   # @getfd:
>>   #
>>   # Receive a file descriptor via SCM rights and assign it a name
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index e3cf3c5..c039947 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -869,6 +869,40 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "pass-fd",
>> +        .args_type  = "fdname:s",
>> +        .params     = "pass-fd name",
>> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
>> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
>> +    },
>> +
>> +SQMP
>> +pass-fd
>> +-------
>> +
>> +Pass a file descriptor via SCM rights and assign it a name.
>> +
>> +Arguments:
>> +
>> +- "fdname": file descriptor name (json-string)
>> +
>> +Return a json-int with the QEMU file descriptor that was received.
>> +
>> +Example:
>> +
>> +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
>> +<- { "return": 42 }
>> +
>> +Notes:
>> +
>> +(1) If the name specified by the "fdname" argument already exists,
>> +    the command will fail.
>> +(2) The 'closefd' command can be used to explicitly close the file
>> +    descriptor when it is no longer needed.
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "getfd",
>>           .args_type  = "fdname:s",
>>           .params     = "getfd name",
>
>

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Luiz Capitulino]

On Fri, 15 Jun 2012 11:04:16 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> 
> 
> On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
> > On Thu, 14 Jun 2012 11:55:02 -0400
> > Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
> >
> >> This patch adds the pass-fd QMP command using the QAPI framework.
> >> Like the getfd command, it is used to pass a file descriptor via
> >> SCM_RIGHTS.  However, the pass-fd command also returns the received
> >> file descriptor, which is a difference in behavior from the getfd
> >> command, which returns nothing.
> >>
> >> The closefd command can be used to close a file descriptor that was
> >> passed with the pass-fd command.
> >>
> >> Note that when using getfd or pass-fd, there are some commands
> >> (e.g. migrate with fd:name) that implicitly close the named fd.
> >> When this is not the case, closefd must be used to avoid fd leaks.
> >>
> >> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> >> ---
> >> v2:
> >>   -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
> >>   -Use passfd as command name (berrange@redhat.com)
> >>
> >> v3:
> >>   -Use pass-fd as command name (lcapitulino@redhat.com)
> >>   -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
> >>   -Add notes to QMP command describing behavior in more detail
> >>    (lcapitulino@redhat.com, eblake@redhat.com)
> >>   -Add note about fd leakage (eblake@redhat.com)
> >>
> >>   monitor.c        |   33 +++++++++++++++++++++++++++++++++
> >>   qapi-schema.json |   19 +++++++++++++++++++
> >>   qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
> >>   3 files changed, 86 insertions(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 1a7f7e7..6d99368 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
> >>   }
> >>   #endif
> >>
> >> +int64_t qmp_pass_fd(const char *fdname, Error **errp)
> >> +{
> >> +    mon_fd_t *monfd;
> >> +    int fd;
> >> +
> >> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
> >> +    if (fd == -1) {
> >> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (qemu_isdigit(fdname[0])) {
> >> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> +                  "a name not starting with a digit");
> >> +        return -1;
> >> +    }
> >> +
> >> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >> +        if (strcmp(monfd->name, fdname) == 0) {
> >> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
> >> +                      "a name that does not already exist");
> >> +            return -1;
> >> +        }
> >> +    }
> >
> > Returning the same error class for two different errors is not a good idea.
> > I think you have two options here. You could return QERR_INVALID_PARAMETER
> > for the "already exists" case or introduce QERR_FD_EXISTS. The later is
> > certainly nicer, but we were trying to avoid having too specific errors...
> >
> 
> I'm not clear on what the problem is with returning the same error class 
> for two different errors.  Could you explain?  I don't have a problem 
> changing it if it's an issue.

Because an mngt app/user won't know if what has happened was that the fd name
is invalid or if the fd name already exists.

> 
> >> +
> >> +    monfd = g_malloc0(sizeof(mon_fd_t));
> >> +    monfd->name = g_strdup(fdname);
> >> +    monfd->fd = fd;
> >
> > Maybe you could try to move this to a separate function to share code with
> > qmp_getfd()?
> >
> 
> Sure, no problem.  I can do that.
> 
> >> +
> >> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
> >> +    return fd;
> >> +}
> >> +
> >>   void qmp_getfd(const char *fdname, Error **errp)
> >>   {
> >>       mon_fd_t *monfd;
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 26a6b84..ed99f23 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1864,6 +1864,25 @@
> >>   { 'command': 'netdev_del', 'data': {'id': 'str'} }
> >>
> >>   ##
> >> +# @pass-fd:
> >> +#
> >> +# Pass a file descriptor via SCM rights and assign it a name
> >> +#
> >> +# @fdname: file descriptor name
> >> +#
> >> +# Returns: The QEMU file descriptor that was received
> >> +#          If file descriptor was not received, FdNotSupplied
> >> +#          If @fdname is not valid, InvalidParameterType
> >> +#
> >> +# Since: 1.2.0
> >> +#
> >> +# Notes: If @fdname already exists, the command will fail.
> >> +#        The 'closefd' command can be used to explicitly close the
> >> +#        file descriptor when it is no longer needed.
> >> +##
> >> +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
> >> +
> >> +##
> >>   # @getfd:
> >>   #
> >>   # Receive a file descriptor via SCM rights and assign it a name
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index e3cf3c5..c039947 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -869,6 +869,40 @@ Example:
> >>   EQMP
> >>
> >>       {
> >> +        .name       = "pass-fd",
> >> +        .args_type  = "fdname:s",
> >> +        .params     = "pass-fd name",
> >> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
> >> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
> >> +    },
> >> +
> >> +SQMP
> >> +pass-fd
> >> +-------
> >> +
> >> +Pass a file descriptor via SCM rights and assign it a name.
> >> +
> >> +Arguments:
> >> +
> >> +- "fdname": file descriptor name (json-string)
> >> +
> >> +Return a json-int with the QEMU file descriptor that was received.
> >> +
> >> +Example:
> >> +
> >> +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
> >> +<- { "return": 42 }
> >> +
> >> +Notes:
> >> +
> >> +(1) If the name specified by the "fdname" argument already exists,
> >> +    the command will fail.
> >> +(2) The 'closefd' command can be used to explicitly close the file
> >> +    descriptor when it is no longer needed.
> >> +
> >> +EQMP
> >> +
> >> +    {
> >>           .name       = "getfd",
> >>           .args_type  = "fdname:s",
> >>           .params     = "getfd name",
> >
> >
> 

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Corey Bryant]

On 06/15/2012 11:14 AM, Luiz Capitulino wrote:
> On Fri, 15 Jun 2012 11:04:16 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>>
>>
>> On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
>>> On Thu, 14 Jun 2012 11:55:02 -0400
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>>
>>>> This patch adds the pass-fd QMP command using the QAPI framework.
>>>> Like the getfd command, it is used to pass a file descriptor via
>>>> SCM_RIGHTS.  However, the pass-fd command also returns the received
>>>> file descriptor, which is a difference in behavior from the getfd
>>>> command, which returns nothing.
>>>>
>>>> The closefd command can be used to close a file descriptor that was
>>>> passed with the pass-fd command.
>>>>
>>>> Note that when using getfd or pass-fd, there are some commands
>>>> (e.g. migrate with fd:name) that implicitly close the named fd.
>>>> When this is not the case, closefd must be used to avoid fd leaks.
>>>>
>>>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>>>> ---
>>>> v2:
>>>>    -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
>>>>    -Use passfd as command name (berrange@redhat.com)
>>>>
>>>> v3:
>>>>    -Use pass-fd as command name (lcapitulino@redhat.com)
>>>>    -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
>>>>    -Add notes to QMP command describing behavior in more detail
>>>>     (lcapitulino@redhat.com, eblake@redhat.com)
>>>>    -Add note about fd leakage (eblake@redhat.com)
>>>>
>>>>    monitor.c        |   33 +++++++++++++++++++++++++++++++++
>>>>    qapi-schema.json |   19 +++++++++++++++++++
>>>>    qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 86 insertions(+)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 1a7f7e7..6d99368 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>>>>    }
>>>>    #endif
>>>>
>>>> +int64_t qmp_pass_fd(const char *fdname, Error **errp)
>>>> +{
>>>> +    mon_fd_t *monfd;
>>>> +    int fd;
>>>> +
>>>> +    fd = qemu_chr_fe_get_msgfd(cur_mon->chr);
>>>> +    if (fd == -1) {
>>>> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    if (qemu_isdigit(fdname[0])) {
>>>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>>>> +                  "a name not starting with a digit");
>>>> +        return -1;
>>>> +    }
>>>> +
>>>> +    QLIST_FOREACH(monfd, &cur_mon->fds, next) {
>>>> +        if (strcmp(monfd->name, fdname) == 0) {
>>>> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>>>> +                      "a name that does not already exist");
>>>> +            return -1;
>>>> +        }
>>>> +    }
>>>
>>> Returning the same error class for two different errors is not a good idea.
>>> I think you have two options here. You could return QERR_INVALID_PARAMETER
>>> for the "already exists" case or introduce QERR_FD_EXISTS. The later is
>>> certainly nicer, but we were trying to avoid having too specific errors...
>>>
>>
>> I'm not clear on what the problem is with returning the same error class
>> for two different errors.  Could you explain?  I don't have a problem
>> changing it if it's an issue.
>
> Because an mngt app/user won't know if what has happened was that the fd name
> is invalid or if the fd name already exists.
>

I agree, it makes sense so the management app can program based on the 
error class.  It just seems like the error classes in general should be 
able to be used multiple times if it fits, especially if we're trying to 
avoid having too specific errors.

Maybe (in the future) something like a sub-class could be used? In other 
words to allow using the same error class with unique sub-classes.

I'll plan on updating to use a different error class for this.

>>
>>>> +
>>>> +    monfd = g_malloc0(sizeof(mon_fd_t));
>>>> +    monfd->name = g_strdup(fdname);
>>>> +    monfd->fd = fd;
>>>
>>> Maybe you could try to move this to a separate function to share code with
>>> qmp_getfd()?
>>>
>>
>> Sure, no problem.  I can do that.
>>
>>>> +
>>>> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>>>> +    return fd;
>>>> +}
>>>> +
>>>>    void qmp_getfd(const char *fdname, Error **errp)
>>>>    {
>>>>        mon_fd_t *monfd;
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 26a6b84..ed99f23 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -1864,6 +1864,25 @@
>>>>    { 'command': 'netdev_del', 'data': {'id': 'str'} }
>>>>
>>>>    ##
>>>> +# @pass-fd:
>>>> +#
>>>> +# Pass a file descriptor via SCM rights and assign it a name
>>>> +#
>>>> +# @fdname: file descriptor name
>>>> +#
>>>> +# Returns: The QEMU file descriptor that was received
>>>> +#          If file descriptor was not received, FdNotSupplied
>>>> +#          If @fdname is not valid, InvalidParameterType
>>>> +#
>>>> +# Since: 1.2.0
>>>> +#
>>>> +# Notes: If @fdname already exists, the command will fail.
>>>> +#        The 'closefd' command can be used to explicitly close the
>>>> +#        file descriptor when it is no longer needed.
>>>> +##
>>>> +{ 'command': 'pass-fd', 'data': {'fdname': 'str'}, 'returns': 'int' }
>>>> +
>>>> +##
>>>>    # @getfd:
>>>>    #
>>>>    # Receive a file descriptor via SCM rights and assign it a name
>>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>>> index e3cf3c5..c039947 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -869,6 +869,40 @@ Example:
>>>>    EQMP
>>>>
>>>>        {
>>>> +        .name       = "pass-fd",
>>>> +        .args_type  = "fdname:s",
>>>> +        .params     = "pass-fd name",
>>>> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +pass-fd
>>>> +-------
>>>> +
>>>> +Pass a file descriptor via SCM rights and assign it a name.
>>>> +
>>>> +Arguments:
>>>> +
>>>> +- "fdname": file descriptor name (json-string)
>>>> +
>>>> +Return a json-int with the QEMU file descriptor that was received.
>>>> +
>>>> +Example:
>>>> +
>>>> +-> { "execute": "pass-fd", "arguments": { "fdname": "fd1" } }
>>>> +<- { "return": 42 }
>>>> +
>>>> +Notes:
>>>> +
>>>> +(1) If the name specified by the "fdname" argument already exists,
>>>> +    the command will fail.
>>>> +(2) The 'closefd' command can be used to explicitly close the file
>>>> +    descriptor when it is no longer needed.
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>            .name       = "getfd",
>>>>            .args_type  = "fdname:s",
>>>>            .params     = "getfd name",
>>>
>>>
>>
>

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

[Luiz Capitulino]

On Fri, 15 Jun 2012 11:29:03 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> >> I'm not clear on what the problem is with returning the same error class
> >> for two different errors.  Could you explain?  I don't have a problem
> >> changing it if it's an issue.
> >
> > Because an mngt app/user won't know if what has happened was that the fd name
> > is invalid or if the fd name already exists.
> >
> 
> I agree, it makes sense so the management app can program based on the 
> error class.  It just seems like the error classes in general should be 
> able to be used multiple times if it fits, especially if we're trying to 
> avoid having too specific errors.
> 
> Maybe (in the future) something like a sub-class could be used? In other 
> words to allow using the same error class with unique sub-classes.

There's a discussion going on which is more or less the same topic:

 http://lists.gnu.org/archive/html/qemu-devel/2012-06/msg02243.html

[Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

This patch adds support to qemu_open to dup(fd) a pre-opened file
descriptor if the filename is of the format /dev/fd/X.

This can be used when QEMU is restricted from opening files, and
the management application opens files on QEMU's behalf.

If the fd was passed to the monitor with the pass-fd command, it
must be explicitly closed with the 'closefd' command when it is
no longer required, in order to prevent fd leaks.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2:
 -Get rid of file_open and move dup code to qemu_open
  (kwolf@redhat.com)
 -Use strtol wrapper instead of atoi (kwolf@redhat.com)

v3:
 -Add note about fd leakage (eblake@redhat.com)

 osdep.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/osdep.c b/osdep.c
index 3e6bada..c17cdcb 100644
--- a/osdep.c
+++ b/osdep.c
@@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
     int ret;
     int mode = 0;
 
+#ifndef _WIN32
+    const char *p;
+
+    /* Attempt dup of fd for pre-opened file */
+    if (strstart(name, "/dev/fd/", &p)) {
+        ret = qemu_parse_fd(p);
+        if (ret == -1) {
+            return -1;
+        }
+        return dup(ret);
+    }
+#endif
+
     if (flags & O_CREAT) {
         va_list ap;
 
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2405 bytes --]

On 06/14/2012 09:55 AM, Corey Bryant wrote:
> This patch adds support to qemu_open to dup(fd) a pre-opened file
> descriptor if the filename is of the format /dev/fd/X.
> 

> +++ b/osdep.c
> @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
>      int ret;
>      int mode = 0;
>  
> +#ifndef _WIN32
> +    const char *p;
> +
> +    /* Attempt dup of fd for pre-opened file */
> +    if (strstart(name, "/dev/fd/", &p)) {
> +        ret = qemu_parse_fd(p);
> +        if (ret == -1) {
> +            return -1;
> +        }
> +        return dup(ret);

I think you need to honor flags so that the end use of the fd will be as
if qemu had directly opened the file, rather than just doing a blind dup
with a resulting fd that is in a different state than the caller
expected.  I can think of at least the following cases (there may be more):

if (flags & O_TRUNCATE ||
      ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL))
    ftruncate(ret, 0);

Why do I truncate on O_CREAT|O_EXCL?  Because that's a case where open()
guarantees that the file will have just been created empty.

if (flags & O_CLOEXEC)
   use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible
   else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically

Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
why 'migrate fd:name' would need to be inheritable, and in the case of
/dev/fd/ parsing, while the dup() result may need to be inheritable, the
original that we are dup'ing from should most certainly be cloexec.

if (flags & O_NONBLOCK)
   use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
else
   use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK

or maybe we document that callers of pass-fd must always pass fds with
O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
sure part of the process of tying name with fd in the lookup list of
named fds is determining the current O_NONBLOCK state in case future
qemu_open() need it in the opposite state.

Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to
match the desired open() setting).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/15/2012 11:16 AM, Eric Blake wrote:
> On 06/14/2012 09:55 AM, Corey Bryant wrote:
>> This patch adds support to qemu_open to dup(fd) a pre-opened file
>> descriptor if the filename is of the format /dev/fd/X.
>>
>
>> +++ b/osdep.c
>> @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
>>       int ret;
>>       int mode = 0;
>>
>> +#ifndef _WIN32
>> +    const char *p;
>> +
>> +    /* Attempt dup of fd for pre-opened file */
>> +    if (strstart(name, "/dev/fd/", &p)) {
>> +        ret = qemu_parse_fd(p);
>> +        if (ret == -1) {
>> +            return -1;
>> +        }
>> +        return dup(ret);
>
> I think you need to honor flags so that the end use of the fd will be as
> if qemu had directly opened the file, rather than just doing a blind dup
> with a resulting fd that is in a different state than the caller
> expected.  I can think of at least the following cases (there may be more):

I was thinking libvirt would handle all the flag settings on open 
(obviously since that's how I coded it).  I think you're right with this 
approach though as QEMU will re-open the same file various times with 
different flags.

There are some flags that I don't think we'll be able to change.  For 
example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all 
files O_RDWR.

>
> if (flags & O_TRUNCATE ||
>        ((flags & (O_CREAT|O_EXCL)) == (O_CREAT|O_EXCL))
>      ftruncate(ret, 0);
>
> Why do I truncate on O_CREAT|O_EXCL?  Because that's a case where open()
> guarantees that the file will have just been created empty.
>

That makes sense.

> if (flags & O_CLOEXEC)
>     use fcntl(F_DUPFD_CLOEXEC) instead of dup(), if possible
>     else fallback to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC non-atomically
>

That makes sense too.

> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
> why 'migrate fd:name' would need to be inheritable, and in the case of
> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
> original that we are dup'ing from should most certainly be cloexec.

It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't 
think we can modify getfd at this point (compatibility) but we could 
update pass-fd to set it.  It may make more sense to set it with 
fcntl(FD_CLOEXEC) in qmp_pass_fd().

>
> if (flags & O_NONBLOCK)
>     use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
> else
>     use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>
> or maybe we document that callers of pass-fd must always pass fds with
> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
> sure part of the process of tying name with fd in the lookup list of
> named fds is determining the current O_NONBLOCK state in case future
> qemu_open() need it in the opposite state.

Just documenting it seems error-prone.  Why not just set/clear it based 
on the flag passed to qemu_open?

>
> Treat O_APPEND similarly to O_NONBLOCK (with fcntl(F_GETFL/F_SETFL) to
> match the desired open() setting).
>

Ok

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 4466 bytes --]

On 06/15/2012 12:16 PM, Corey Bryant wrote:

>> I think you need to honor flags so that the end use of the fd will be as
>> if qemu had directly opened the file, rather than just doing a blind dup
>> with a resulting fd that is in a different state than the caller
>> expected.  I can think of at least the following cases (there may be
>> more):
> 
> I was thinking libvirt would handle all the flag settings on open
> (obviously since that's how I coded it).  I think you're right with this
> approach though as QEMU will re-open the same file various times with
> different flags.

How will libvirt know whether qemu wanted to open a file with O_TRUNCATE
vs. reusing an entire file, or with O_NONBLOCK or not, and so forth?  I
think there are just too many qmeu_open() calls with different flags to
expect libvirt to know how to pre-open all possible fds in such a manner
that /dev/fd/nnn will be a valid replacement for what qemu would have
done, in the cases where qemu can instead correct flags itself.

> 
> There are some flags that I don't think we'll be able to change.  For
> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
> files O_RDWR.

I agree that this can't be changed, so this is one case where libvirt
will have to know what the file will used for.  But it is also a case
where qemu can at least check whether the fd passed in has sufficient
permissions (fcntl(F_GETFL)) for what qemu wanted to use, and should
error out here rather than silently succeed here then have a weird EBADF
failure down the road when the dup'd fd is finally used.  You are right
that libvirt should always be safe in passing in an O_RDWR fd for
whatever qemu wants, although that may represent security holes (there's
reasons for O_RDONLY); and in cases where libvirt is instead passing in
one end of a pipe, the fd will necessarily be O_RDONLY or O_WRONLY
(since you can't have an O_RDWR pipe).

>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
>> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
>> why 'migrate fd:name' would need to be inheritable, and in the case of
>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
>> original that we are dup'ing from should most certainly be cloexec.
> 
> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't
> think we can modify getfd at this point (compatibility) but we could
> update pass-fd to set it.  It may make more sense to set it with
> fcntl(FD_CLOEXEC) in qmp_pass_fd().

That's not atomic.  If we don't care about atomicity (for example, if we
can guarantee that no other thread in qemu can possibly fork/exec in
between the monitor thread receiving an fd then converting it to
cloexec, based on how we hold a mutex), then that's fine.  OR, if we
make sure _all_ fork() calls sanitize themselves, by closing all fds in
the getfd/pass-fd list prior to calling exec(), then we don't have to
even worry about cloexec (but then you have to worry about locking the
getfd name list, since locking a list to iterate it is not an async-safe
action and probably can't be done between fork() and exec()).
Otherwise, using MSG_CMSG_CLOEXEC is the only safe way to avoid leaking
unintended fds into another thread's child process.

> 
>>
>> if (flags & O_NONBLOCK)
>>     use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
>> else
>>     use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>>
>> or maybe we document that callers of pass-fd must always pass fds with
>> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
>> sure part of the process of tying name with fd in the lookup list of
>> named fds is determining the current O_NONBLOCK state in case future
>> qemu_open() need it in the opposite state.
> 
> Just documenting it seems error-prone.  Why not just set/clear it based
> on the flag passed to qemu_open?

Yep, back to my argument that making libvirt have to know every context
that qemu will want to open, and what flags it would be using in those
contexts, is painful compared to having qemu just do the right thing in
the first place, or gracefully erroring out right away at the point of
the qemu_open(/dev/fd/nnn).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/15/2012 02:42 PM, Eric Blake wrote:
> On 06/15/2012 12:16 PM, Corey Bryant wrote:
>
>>> I think you need to honor flags so that the end use of the fd will be as
>>> if qemu had directly opened the file, rather than just doing a blind dup
>>> with a resulting fd that is in a different state than the caller
>>> expected.  I can think of at least the following cases (there may be
>>> more):
>>
>> I was thinking libvirt would handle all the flag settings on open
>> (obviously since that's how I coded it).  I think you're right with this
>> approach though as QEMU will re-open the same file various times with
>> different flags.
>
> How will libvirt know whether qemu wanted to open a file with O_TRUNCATE
> vs. reusing an entire file, or with O_NONBLOCK or not, and so forth?  I
> think there are just too many qmeu_open() calls with different flags to
> expect libvirt to know how to pre-open all possible fds in such a manner
> that /dev/fd/nnn will be a valid replacement for what qemu would have
> done, in the cases where qemu can instead correct flags itself.
>

I agree completely.

>>
>> There are some flags that I don't think we'll be able to change.  For
>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>> files O_RDWR.
>
> I agree that this can't be changed, so this is one case where libvirt
> will have to know what the file will used for.  But it is also a case
> where qemu can at least check whether the fd passed in has sufficient
> permissions (fcntl(F_GETFL)) for what qemu wanted to use, and should
> error out here rather than silently succeed here then have a weird EBADF
> failure down the road when the dup'd fd is finally used.  You are right
> that libvirt should always be safe in passing in an O_RDWR fd for
> whatever qemu wants, although that may represent security holes (there's
> reasons for O_RDONLY); and in cases where libvirt is instead passing in
> one end of a pipe, the fd will necessarily be O_RDONLY or O_WRONLY
> (since you can't have an O_RDWR pipe).

Good points.  In addition to flag setting, I'll add some flag checking 
for those flags that can't be set (ie. O_RDONLY, O_WRONLY).

>
>>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
>>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
>>> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
>>> why 'migrate fd:name' would need to be inheritable, and in the case of
>>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
>>> original that we are dup'ing from should most certainly be cloexec.
>>
>> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't
>> think we can modify getfd at this point (compatibility) but we could
>> update pass-fd to set it.  It may make more sense to set it with
>> fcntl(FD_CLOEXEC) in qmp_pass_fd().
>
> That's not atomic.  If we don't care about atomicity (for example, if we
> can guarantee that no other thread in qemu can possibly fork/exec in
> between the monitor thread receiving an fd then converting it to
> cloexec, based on how we hold a mutex), then that's fine.  OR, if we
> make sure _all_ fork() calls sanitize themselves, by closing all fds in
> the getfd/pass-fd list prior to calling exec(), then we don't have to
> even worry about cloexec (but then you have to worry about locking the
> getfd name list, since locking a list to iterate it is not an async-safe
> action and probably can't be done between fork() and exec()).
> Otherwise, using MSG_CMSG_CLOEXEC is the only safe way to avoid leaking
> unintended fds into another thread's child process.

Ok I'm sold.  I'll first look into setting MSG_CMSG_CLOEXEC.

>
>>
>>>
>>> if (flags & O_NONBLOCK)
>>>      use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
>>> else
>>>      use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>>>
>>> or maybe we document that callers of pass-fd must always pass fds with
>>> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
>>> sure part of the process of tying name with fd in the lookup list of
>>> named fds is determining the current O_NONBLOCK state in case future
>>> qemu_open() need it in the opposite state.
>>
>> Just documenting it seems error-prone.  Why not just set/clear it based
>> on the flag passed to qemu_open?
>
> Yep, back to my argument that making libvirt have to know every context
> that qemu will want to open, and what flags it would be using in those
> contexts, is painful compared to having qemu just do the right thing in
> the first place, or gracefully erroring out right away at the point of
> the qemu_open(/dev/fd/nnn).
>

I agree.

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Kevin Wolf]

Am 15.06.2012 20:16, schrieb Corey Bryant:
> 
> 
> On 06/15/2012 11:16 AM, Eric Blake wrote:
>> On 06/14/2012 09:55 AM, Corey Bryant wrote:
>>> This patch adds support to qemu_open to dup(fd) a pre-opened file
>>> descriptor if the filename is of the format /dev/fd/X.
>>>
>>
>>> +++ b/osdep.c
>>> @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
>>>       int ret;
>>>       int mode = 0;
>>>
>>> +#ifndef _WIN32
>>> +    const char *p;
>>> +
>>> +    /* Attempt dup of fd for pre-opened file */
>>> +    if (strstart(name, "/dev/fd/", &p)) {
>>> +        ret = qemu_parse_fd(p);
>>> +        if (ret == -1) {
>>> +            return -1;
>>> +        }
>>> +        return dup(ret);
>>
>> I think you need to honor flags so that the end use of the fd will be as
>> if qemu had directly opened the file, rather than just doing a blind dup
>> with a resulting fd that is in a different state than the caller
>> expected.  I can think of at least the following cases (there may be more):
> 
> I was thinking libvirt would handle all the flag settings on open 
> (obviously since that's how I coded it).  I think you're right with this 
> approach though as QEMU will re-open the same file various times with 
> different flags.
> 
> There are some flags that I don't think we'll be able to change.  For 
> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all 
> files O_RDWR.

I think we need to check all of them and fail qemu_open() if they don't
match. Those that qemu can change, should be just changed, of course.

>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
>> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
>> why 'migrate fd:name' would need to be inheritable, and in the case of
>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
>> original that we are dup'ing from should most certainly be cloexec.
> 
> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't 
> think we can modify getfd at this point (compatibility) but we could 
> update pass-fd to set it.  It may make more sense to set it with 
> fcntl(FD_CLOEXEC) in qmp_pass_fd().

In which scenario would any client break if we set FD_CLOEXEC? I don't
think compatibility means we can't fix any bugs.

>> if (flags & O_NONBLOCK)
>>     use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
>> else
>>     use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>>
>> or maybe we document that callers of pass-fd must always pass fds with
>> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
>> sure part of the process of tying name with fd in the lookup list of
>> named fds is determining the current O_NONBLOCK state in case future
>> qemu_open() need it in the opposite state.
> 
> Just documenting it seems error-prone.  Why not just set/clear it based 
> on the flag passed to qemu_open?

I agree. We could just check and return an error if they aren't set
correctly, but I think adjusting the flags is nicer.

Kevin

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/15/2012 02:46 PM, Kevin Wolf wrote:
> Am 15.06.2012 20:16, schrieb Corey Bryant:
>>
>>
>> On 06/15/2012 11:16 AM, Eric Blake wrote:
>>> On 06/14/2012 09:55 AM, Corey Bryant wrote:
>>>> This patch adds support to qemu_open to dup(fd) a pre-opened file
>>>> descriptor if the filename is of the format /dev/fd/X.
>>>>
>>>
>>>> +++ b/osdep.c
>>>> @@ -82,6 +82,19 @@ int qemu_open(const char *name, int flags, ...)
>>>>        int ret;
>>>>        int mode = 0;
>>>>
>>>> +#ifndef _WIN32
>>>> +    const char *p;
>>>> +
>>>> +    /* Attempt dup of fd for pre-opened file */
>>>> +    if (strstart(name, "/dev/fd/", &p)) {
>>>> +        ret = qemu_parse_fd(p);
>>>> +        if (ret == -1) {
>>>> +            return -1;
>>>> +        }
>>>> +        return dup(ret);
>>>
>>> I think you need to honor flags so that the end use of the fd will be as
>>> if qemu had directly opened the file, rather than just doing a blind dup
>>> with a resulting fd that is in a different state than the caller
>>> expected.  I can think of at least the following cases (there may be more):
>>
>> I was thinking libvirt would handle all the flag settings on open
>> (obviously since that's how I coded it).  I think you're right with this
>> approach though as QEMU will re-open the same file various times with
>> different flags.
>>
>> There are some flags that I don't think we'll be able to change.  For
>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>> files O_RDWR.
>
> I think we need to check all of them and fail qemu_open() if they don't
> match. Those that qemu can change, should be just changed, of course.
>

Ok.  I remember a scenario where QEMU opens a file read-only (perhaps to 
check headers and determine the file format) before re-opening it 
read-write.  Perhaps this is only when format= isn't specified with 
-drive.  I'm thinking we may need to change flags to read-write where 
they used to be read-only, in some circumstances.

>>> Oh, and are we using MSG_CMSG_CLOEXEC where possible (and where not
>>> possible, falling back to fcntl(F_GETFD/F_SETFD) to set FD_CLOEXEC) on
>>> all fds received by 'getfd' and 'pass-fd'?  I can't think of any reason
>>> why 'migrate fd:name' would need to be inheritable, and in the case of
>>> /dev/fd/ parsing, while the dup() result may need to be inheritable, the
>>> original that we are dup'ing from should most certainly be cloexec.
>>
>> It doesn't look like we use MSG_CMSG_CLOEXEC anywhere in QEMU.  I don't
>> think we can modify getfd at this point (compatibility) but we could
>> update pass-fd to set it.  It may make more sense to set it with
>> fcntl(FD_CLOEXEC) in qmp_pass_fd().
>
> In which scenario would any client break if we set FD_CLOEXEC? I don't
> think compatibility means we can't fix any bugs.
>

I don't know if it breaks any client.  Maybe it's not a compatibility 
error.  It dopes change behavior down the line though.  If you think 
it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.

>>> if (flags & O_NONBLOCK)
>>>      use fcntl(F_GETFL/F_SETFL) to set O_NONBLOCK
>>> else
>>>      use fcntl(F_GETFL/F_SETFL) to clear O_NONBLOCK
>>>
>>> or maybe we document that callers of pass-fd must always pass fds with
>>> O_NONBLOCK clear instead of clearing it ourselves.  Or maybe we make
>>> sure part of the process of tying name with fd in the lookup list of
>>> named fds is determining the current O_NONBLOCK state in case future
>>> qemu_open() need it in the opposite state.
>>
>> Just documenting it seems error-prone.  Why not just set/clear it based
>> on the flag passed to qemu_open?
>
> I agree. We could just check and return an error if they aren't set
> correctly, but I think adjusting the flags is nicer.
>
> Kevin
>

Ok thanks for the input!

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]

On 06/15/2012 01:19 PM, Corey Bryant wrote:

>>> There are some flags that I don't think we'll be able to change.  For
>>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>>> files O_RDWR.
>>
>> I think we need to check all of them and fail qemu_open() if they don't
>> match. Those that qemu can change, should be just changed, of course.
>>
> 
> Ok.  I remember a scenario where QEMU opens a file read-only (perhaps to
> check headers and determine the file format) before re-opening it
> read-write.  Perhaps this is only when format= isn't specified with
> -drive.  I'm thinking we may need to change flags to read-write where
> they used to be read-only, in some circumstances.

In those situations, libvirt would pass fd with O_RDWR, and qemu_open()
would be fine requesting O_RDONLY the first time (subset is okay), and
O_RDWR the second time.  Where you have to error out is where libvirt
passes O_RDONLY but qemu wants O_RDWR, and so forth.


>>
>> In which scenario would any client break if we set FD_CLOEXEC? I don't
>> think compatibility means we can't fix any bugs.
>>
> 
> I don't know if it breaks any client.  Maybe it's not a compatibility
> error.  It dopes change behavior down the line though.  If you think
> it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.

The only case that a client might break is if there were a way to pass
an fd into qemu and then intentionally see that fd in a child process of
qemu.  But in the case of 'migrate fd:nnn', you aren't spawning a child
process, and even in the case of 'migrate exec:command' (which libvirt
no longer uses if fd:nnn works), I don't see how the client could have
ever intentionally tried to use 'getfd' in advance to pass an extra fd
for use inside the 'exec:command' child.  Besides, before 'pass-fd' was
around, how would the management app triggering the 'exec:command' even
know what fd number would accidentally be inherited into the
exec:command child?  I think it is pretty much a straight bug-fix for
'getfd' to always set FD_CLOEXEC, and preferably set it atomically via
MSG_CMSG_CLOEXEC.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/15/2012 04:00 PM, Eric Blake wrote:
> On 06/15/2012 01:19 PM, Corey Bryant wrote:
>
>>>> There are some flags that I don't think we'll be able to change.  For
>>>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>>>> files O_RDWR.
>>>
>>> I think we need to check all of them and fail qemu_open() if they don't
>>> match. Those that qemu can change, should be just changed, of course.
>>>
>>
>> Ok.  I remember a scenario where QEMU opens a file read-only (perhaps to
>> check headers and determine the file format) before re-opening it
>> read-write.  Perhaps this is only when format= isn't specified with
>> -drive.  I'm thinking we may need to change flags to read-write where
>> they used to be read-only, in some circumstances.
>
> In those situations, libvirt would pass fd with O_RDWR, and qemu_open()
> would be fine requesting O_RDONLY the first time (subset is okay), and
> O_RDWR the second time.  Where you have to error out is where libvirt
> passes O_RDONLY but qemu wants O_RDWR, and so forth.
>

I'll plan on going with this approach.

>
>>>
>>> In which scenario would any client break if we set FD_CLOEXEC? I don't
>>> think compatibility means we can't fix any bugs.
>>>
>>
>> I don't know if it breaks any client.  Maybe it's not a compatibility
>> error.  It dopes change behavior down the line though.  If you think

s/dopes/does

>> it's ok to set FD_CLOEXEC for getfd too, then I'm happy to do it.
>
> The only case that a client might break is if there were a way to pass
> an fd into qemu and then intentionally see that fd in a child process of
> qemu.  But in the case of 'migrate fd:nnn', you aren't spawning a child
> process, and even in the case of 'migrate exec:command' (which libvirt
> no longer uses if fd:nnn works), I don't see how the client could have
> ever intentionally tried to use 'getfd' in advance to pass an extra fd
> for use inside the 'exec:command' child.  Besides, before 'pass-fd' was
> around, how would the management app triggering the 'exec:command' even
> know what fd number would accidentally be inherited into the
> exec:command child?  I think it is pretty much a straight bug-fix for
> 'getfd' to always set FD_CLOEXEC, and preferably set it atomically via
> MSG_CMSG_CLOEXEC.
>

Alright, I'll go ahead and make this update in the next version of the 
patch series.

Thanks for all the input!

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Kevin Wolf]

Am 15.06.2012 22:00, schrieb Eric Blake:
> On 06/15/2012 01:19 PM, Corey Bryant wrote:
> 
>>>> There are some flags that I don't think we'll be able to change.  For
>>>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>>>> files O_RDWR.
>>>
>>> I think we need to check all of them and fail qemu_open() if they don't
>>> match. Those that qemu can change, should be just changed, of course.
>>>
>>
>> Ok.  I remember a scenario where QEMU opens a file read-only (perhaps to
>> check headers and determine the file format) before re-opening it
>> read-write.  Perhaps this is only when format= isn't specified with
>> -drive.  I'm thinking we may need to change flags to read-write where
>> they used to be read-only, in some circumstances.
> 
> In those situations, libvirt would pass fd with O_RDWR, and qemu_open()
> would be fine requesting O_RDONLY the first time (subset is okay), and
> O_RDWR the second time.  Where you have to error out is where libvirt
> passes O_RDONLY but qemu wants O_RDWR, and so forth.

Let's try it with requiring an exact match first. If you pass the
format, I think the probing is completely avoided indeed, and having
read-only images really opened O_RDONLY protects against stupid mistakes.

Or if we really need to open the file for probing, maybe we could add a
flag that relaxes the check and that isn't used in the real bdrv_open().

Kevin

Re: [Qemu-devel] [PATCH v3 3/5] osdep: Enable qemu_open to dup pre-opened fd

[Corey Bryant]

On 06/18/2012 04:10 AM, Kevin Wolf wrote:
> Am 15.06.2012 22:00, schrieb Eric Blake:
>> On 06/15/2012 01:19 PM, Corey Bryant wrote:
>>
>>>>> There are some flags that I don't think we'll be able to change.  For
>>>>> example: O_RDONLY, O_WRONLY, O_RDWR.  I assume libvirt would open all
>>>>> files O_RDWR.
>>>>
>>>> I think we need to check all of them and fail qemu_open() if they don't
>>>> match. Those that qemu can change, should be just changed, of course.
>>>>
>>>
>>> Ok.  I remember a scenario where QEMU opens a file read-only (perhaps to
>>> check headers and determine the file format) before re-opening it
>>> read-write.  Perhaps this is only when format= isn't specified with
>>> -drive.  I'm thinking we may need to change flags to read-write where
>>> they used to be read-only, in some circumstances.
>>
>> In those situations, libvirt would pass fd with O_RDWR, and qemu_open()
>> would be fine requesting O_RDONLY the first time (subset is okay), and
>> O_RDWR the second time.  Where you have to error out is where libvirt
>> passes O_RDONLY but qemu wants O_RDWR, and so forth.
>
> Let's try it with requiring an exact match first. If you pass the
> format, I think the probing is completely avoided indeed, and having
> read-only images really opened O_RDONLY protects against stupid mistakes.
>
> Or if we really need to open the file for probing, maybe we could add a
> flag that relaxes the check and that isn't used in the real bdrv_open().
>
> Kevin
>

I haven't heard any objection to this so I'll be checking for exact 
match, and implementing a flag to relax the check only if it's necessary.

-- 
Regards,
Corey

[Qemu-devel] [PATCH v3 4/5] block: Convert open calls to qemu_open

[Corey Bryant]

This patch converts all block layer open calls to qemu_open.  This
enables all block layer open paths to dup(X) a pre-opened file
descriptor if the filename is of the format /dev/fd/X.  This is
useful if QEMU is restricted from opening certain files.

Note that this adds the O_CLOEXEC flag to the changed open paths
when the O_CLOEXEC macro is defined.

Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v2:
 -Convert calls to qemu_open instead of file_open (kwolf@redhat.com)
 -Mention introduction of O_CLOEXEC (kwolf@redhat.com)

v3:
 -No changes

 block/raw-posix.c |   18 +++++++++---------
 block/raw-win32.c |    4 ++--
 block/vdi.c       |    5 +++--
 block/vmdk.c      |   21 +++++++++------------
 block/vpc.c       |    2 +-
 block/vvfat.c     |   21 +++++++++++----------
 6 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 03fcfcc..d8eff2f 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
-              0644);
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+                   0644);
     if (fd < 0) {
         result = -errno;
     } else {
@@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
         if ( bsdPath[ 0 ] != '\0' ) {
             strcat(bsdPath,"s0");
             /* some CDs don't have a partition 0 */
-            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
+            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
             if (fd < 0) {
                 bsdPath[strlen(bsdPath)-1] = '1';
             } else {
@@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs)
 #endif
             return -EIO;
         }
-        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
+        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
         if (s->fd < 0) {
             s->fd_error_time = get_clock();
             s->fd_got_error = 1;
@@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_BINARY);
+    fd = qemu_open(filename, O_WRONLY | O_BINARY);
     if (fd < 0)
         return -errno;
 
@@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename)
     if (strstart(filename, "/dev/fd", NULL))
         prio = 50;
 
-    fd = open(filename, O_RDONLY | O_NONBLOCK);
+    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
         goto out;
     }
@@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag)
         close(s->fd);
         s->fd = -1;
     }
-    fd = open(bs->filename, s->open_flags | O_NONBLOCK);
+    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
     if (fd >= 0) {
         if (ioctl(fd, FDEJECT, 0) < 0)
             perror("FDEJECT");
@@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename)
     int prio = 0;
     struct stat st;
 
-    fd = open(filename, O_RDONLY | O_NONBLOCK);
+    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
         goto out;
     }
@@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs)
      */
     if (s->fd >= 0)
         close(s->fd);
-    fd = open(bs->filename, s->open_flags, 0644);
+    fd = qemu_open(bs->filename, s->open_flags, 0644);
     if (fd < 0) {
         s->fd = -1;
         return -EIO;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index e4b0b75..8d7838d 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
-              0644);
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
+                   0644);
     if (fd < 0)
         return -EIO;
     set_sparse(fd);
diff --git a/block/vdi.c b/block/vdi.c
index 119d3c7..6183fdf 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
         options++;
     }
 
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
-              0644);
+    fd = qemu_open(filename,
+                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
+                   0644);
     if (fd < 0) {
         return -errno;
     }
diff --git a/block/vmdk.c b/block/vmdk.c
index 18e9b4c..557dc1b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     VMDK4Header header;
     uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
 
-    fd = open(
-        filename,
-        O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
-        0644);
+    fd = qemu_open(filename,
+                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
+                   0644);
     if (fd < 0) {
         return -errno;
     }
@@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
             (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
             total_size / (int64_t)(63 * 16 * 512));
     if (split || flat) {
-        fd = open(
-                filename,
-                O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
-                0644);
+        fd = qemu_open(filename,
+                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
+                       0644);
     } else {
-        fd = open(
-                filename,
-                O_WRONLY | O_BINARY | O_LARGEFILE,
-                0644);
+        fd = qemu_open(filename,
+                       O_WRONLY | O_BINARY | O_LARGEFILE,
+                       0644);
     }
     if (fd < 0) {
         return -errno;
diff --git a/block/vpc.c b/block/vpc.c
index 5cd13d1..60ebf5a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
     }
 
     /* Create the file */
-    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
+    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
     if (fd < 0) {
         return -EIO;
     }
diff --git a/block/vvfat.c b/block/vvfat.c
index 0fd3367..81c3a19 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_
 static int open_file(BDRVVVFATState* s,mapping_t* mapping)
 {
     if(!mapping)
-	return -1;
+        return -1;
     if(!s->current_mapping ||
-	    strcmp(s->current_mapping->path,mapping->path)) {
-	/* open file */
-	int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
-	if(fd<0)
-	    return -1;
-	vvfat_close_current_file(s);
-	s->current_fd = fd;
-	s->current_mapping = mapping;
+            strcmp(s->current_mapping->path, mapping->path)) {
+        /* open file */
+        int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
+        if (fd < 0) {
+            return -1;
+        }
+        vvfat_close_current_file(s);
+        s->current_fd = fd;
+        s->current_mapping = mapping;
     }
     return 0;
 }
@@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s,
     for (i = s->cluster_size; i < offset; i += s->cluster_size)
 	c = modified_fat_get(s, c);
 
-    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
+    fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
     if (fd < 0) {
 	fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
 		strerror(errno), errno);
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH v3 4/5] block: Convert open calls to qemu_open

[Luiz Capitulino]

On Thu, 14 Jun 2012 11:55:04 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> This patch converts all block layer open calls to qemu_open.  This
> enables all block layer open paths to dup(X) a pre-opened file
> descriptor if the filename is of the format /dev/fd/X.  This is
> useful if QEMU is restricted from opening certain files.
> 
> Note that this adds the O_CLOEXEC flag to the changed open paths
> when the O_CLOEXEC macro is defined.

Very minor comment, but I'd exchange this with the previous patch. Because
this change is not only related to the /dev/fd/X feature and also because
as soon as you introduce the /dev/fd/X it will work for all file formats.

> 
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
> ---
> v2:
>  -Convert calls to qemu_open instead of file_open (kwolf@redhat.com)
>  -Mention introduction of O_CLOEXEC (kwolf@redhat.com)
> 
> v3:
>  -No changes
> 
>  block/raw-posix.c |   18 +++++++++---------
>  block/raw-win32.c |    4 ++--
>  block/vdi.c       |    5 +++--
>  block/vmdk.c      |   21 +++++++++------------
>  block/vpc.c       |    2 +-
>  block/vvfat.c     |   21 +++++++++++----------
>  6 files changed, 35 insertions(+), 36 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 03fcfcc..d8eff2f 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> -              0644);
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +                   0644);
>      if (fd < 0) {
>          result = -errno;
>      } else {
> @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>          if ( bsdPath[ 0 ] != '\0' ) {
>              strcat(bsdPath,"s0");
>              /* some CDs don't have a partition 0 */
> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
> +            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>              if (fd < 0) {
>                  bsdPath[strlen(bsdPath)-1] = '1';
>              } else {
> @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs)
>  #endif
>              return -EIO;
>          }
> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
> +        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
>          if (s->fd < 0) {
>              s->fd_error_time = get_clock();
>              s->fd_got_error = 1;
> @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_BINARY);
> +    fd = qemu_open(filename, O_WRONLY | O_BINARY);
>      if (fd < 0)
>          return -errno;
>  
> @@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename)
>      if (strstart(filename, "/dev/fd", NULL))
>          prio = 50;
>  
> -    fd = open(filename, O_RDONLY | O_NONBLOCK);
> +    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>      if (fd < 0) {
>          goto out;
>      }
> @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag)
>          close(s->fd);
>          s->fd = -1;
>      }
> -    fd = open(bs->filename, s->open_flags | O_NONBLOCK);
> +    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
>      if (fd >= 0) {
>          if (ioctl(fd, FDEJECT, 0) < 0)
>              perror("FDEJECT");
> @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename)
>      int prio = 0;
>      struct stat st;
>  
> -    fd = open(filename, O_RDONLY | O_NONBLOCK);
> +    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>      if (fd < 0) {
>          goto out;
>      }
> @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>       */
>      if (s->fd >= 0)
>          close(s->fd);
> -    fd = open(bs->filename, s->open_flags, 0644);
> +    fd = qemu_open(bs->filename, s->open_flags, 0644);
>      if (fd < 0) {
>          s->fd = -1;
>          return -EIO;
> diff --git a/block/raw-win32.c b/block/raw-win32.c
> index e4b0b75..8d7838d 100644
> --- a/block/raw-win32.c
> +++ b/block/raw-win32.c
> @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> -              0644);
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
> +                   0644);
>      if (fd < 0)
>          return -EIO;
>      set_sparse(fd);
> diff --git a/block/vdi.c b/block/vdi.c
> index 119d3c7..6183fdf 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> -              0644);
> +    fd = qemu_open(filename,
> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +                   0644);
>      if (fd < 0) {
>          return -errno;
>      }
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 18e9b4c..557dc1b 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>      VMDK4Header header;
>      uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
>  
> -    fd = open(
> -        filename,
> -        O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> -        0644);
> +    fd = qemu_open(filename,
> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +                   0644);
>      if (fd < 0) {
>          return -errno;
>      }
> @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
>              (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
>              total_size / (int64_t)(63 * 16 * 512));
>      if (split || flat) {
> -        fd = open(
> -                filename,
> -                O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> -                0644);
> +        fd = qemu_open(filename,
> +                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> +                       0644);
>      } else {
> -        fd = open(
> -                filename,
> -                O_WRONLY | O_BINARY | O_LARGEFILE,
> -                0644);
> +        fd = qemu_open(filename,
> +                       O_WRONLY | O_BINARY | O_LARGEFILE,
> +                       0644);
>      }
>      if (fd < 0) {
>          return -errno;
> diff --git a/block/vpc.c b/block/vpc.c
> index 5cd13d1..60ebf5a 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
>      }
>  
>      /* Create the file */
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>      if (fd < 0) {
>          return -EIO;
>      }
> diff --git a/block/vvfat.c b/block/vvfat.c
> index 0fd3367..81c3a19 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_
>  static int open_file(BDRVVVFATState* s,mapping_t* mapping)
>  {
>      if(!mapping)
> -	return -1;
> +        return -1;
>      if(!s->current_mapping ||
> -	    strcmp(s->current_mapping->path,mapping->path)) {
> -	/* open file */
> -	int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
> -	if(fd<0)
> -	    return -1;
> -	vvfat_close_current_file(s);
> -	s->current_fd = fd;
> -	s->current_mapping = mapping;
> +            strcmp(s->current_mapping->path, mapping->path)) {
> +        /* open file */
> +        int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
> +        if (fd < 0) {
> +            return -1;
> +        }
> +        vvfat_close_current_file(s);
> +        s->current_fd = fd;
> +        s->current_mapping = mapping;
>      }
>      return 0;
>  }
> @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s,
>      for (i = s->cluster_size; i < offset; i += s->cluster_size)
>  	c = modified_fat_get(s, c);
>  
> -    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
> +    fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
>      if (fd < 0) {
>  	fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
>  		strerror(errno), errno);

Re: [Qemu-devel] [PATCH v3 4/5] block: Convert open calls to qemu_open

[Corey Bryant]

On 06/15/2012 10:36 AM, Luiz Capitulino wrote:
> On Thu, 14 Jun 2012 11:55:04 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> This patch converts all block layer open calls to qemu_open.  This
>> enables all block layer open paths to dup(X) a pre-opened file
>> descriptor if the filename is of the format /dev/fd/X.  This is
>> useful if QEMU is restricted from opening certain files.
>>
>> Note that this adds the O_CLOEXEC flag to the changed open paths
>> when the O_CLOEXEC macro is defined.
>
> Very minor comment, but I'd exchange this with the previous patch. Because
> this change is not only related to the /dev/fd/X feature and also because
> as soon as you introduce the /dev/fd/X it will work for all file formats.
>

Sure, not a problem.

>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>> v2:
>>   -Convert calls to qemu_open instead of file_open (kwolf@redhat.com)
>>   -Mention introduction of O_CLOEXEC (kwolf@redhat.com)
>>
>> v3:
>>   -No changes
>>
>>   block/raw-posix.c |   18 +++++++++---------
>>   block/raw-win32.c |    4 ++--
>>   block/vdi.c       |    5 +++--
>>   block/vmdk.c      |   21 +++++++++------------
>>   block/vpc.c       |    2 +-
>>   block/vvfat.c     |   21 +++++++++++----------
>>   6 files changed, 35 insertions(+), 36 deletions(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index 03fcfcc..d8eff2f 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -568,8 +568,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>>           options++;
>>       }
>>
>> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>> -              0644);
>> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>> +                   0644);
>>       if (fd < 0) {
>>           result = -errno;
>>       } else {
>> @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>>           if ( bsdPath[ 0 ] != '\0' ) {
>>               strcat(bsdPath,"s0");
>>               /* some CDs don't have a partition 0 */
>> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>> +            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>>               if (fd < 0) {
>>                   bsdPath[strlen(bsdPath)-1] = '1';
>>               } else {
>> @@ -798,7 +798,7 @@ static int fd_open(BlockDriverState *bs)
>>   #endif
>>               return -EIO;
>>           }
>> -        s->fd = open(bs->filename, s->open_flags & ~O_NONBLOCK);
>> +        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
>>           if (s->fd < 0) {
>>               s->fd_error_time = get_clock();
>>               s->fd_got_error = 1;
>> @@ -872,7 +872,7 @@ static int hdev_create(const char *filename, QEMUOptionParameter *options)
>>           options++;
>>       }
>>
>> -    fd = open(filename, O_WRONLY | O_BINARY);
>> +    fd = qemu_open(filename, O_WRONLY | O_BINARY);
>>       if (fd < 0)
>>           return -errno;
>>
>> @@ -950,7 +950,7 @@ static int floppy_probe_device(const char *filename)
>>       if (strstart(filename, "/dev/fd", NULL))
>>           prio = 50;
>>
>> -    fd = open(filename, O_RDONLY | O_NONBLOCK);
>> +    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>>       if (fd < 0) {
>>           goto out;
>>       }
>> @@ -1003,7 +1003,7 @@ static void floppy_eject(BlockDriverState *bs, bool eject_flag)
>>           close(s->fd);
>>           s->fd = -1;
>>       }
>> -    fd = open(bs->filename, s->open_flags | O_NONBLOCK);
>> +    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
>>       if (fd >= 0) {
>>           if (ioctl(fd, FDEJECT, 0) < 0)
>>               perror("FDEJECT");
>> @@ -1053,7 +1053,7 @@ static int cdrom_probe_device(const char *filename)
>>       int prio = 0;
>>       struct stat st;
>>
>> -    fd = open(filename, O_RDONLY | O_NONBLOCK);
>> +    fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>>       if (fd < 0) {
>>           goto out;
>>       }
>> @@ -1177,7 +1177,7 @@ static int cdrom_reopen(BlockDriverState *bs)
>>        */
>>       if (s->fd >= 0)
>>           close(s->fd);
>> -    fd = open(bs->filename, s->open_flags, 0644);
>> +    fd = qemu_open(bs->filename, s->open_flags, 0644);
>>       if (fd < 0) {
>>           s->fd = -1;
>>           return -EIO;
>> diff --git a/block/raw-win32.c b/block/raw-win32.c
>> index e4b0b75..8d7838d 100644
>> --- a/block/raw-win32.c
>> +++ b/block/raw-win32.c
>> @@ -255,8 +255,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
>>           options++;
>>       }
>>
>> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>> -              0644);
>> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY,
>> +                   0644);
>>       if (fd < 0)
>>           return -EIO;
>>       set_sparse(fd);
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 119d3c7..6183fdf 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>>           options++;
>>       }
>>
>> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> -              0644);
>> +    fd = qemu_open(filename,
>> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> +                   0644);
>>       if (fd < 0) {
>>           return -errno;
>>       }
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 18e9b4c..557dc1b 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -1161,10 +1161,9 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
>>       VMDK4Header header;
>>       uint32_t tmp, magic, grains, gd_size, gt_size, gt_count;
>>
>> -    fd = open(
>> -        filename,
>> -        O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> -        0644);
>> +    fd = qemu_open(filename,
>> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> +                   0644);
>>       if (fd < 0) {
>>           return -errno;
>>       }
>> @@ -1484,15 +1483,13 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options)
>>               (flags & BLOCK_FLAG_COMPAT6 ? 6 : 4),
>>               total_size / (int64_t)(63 * 16 * 512));
>>       if (split || flat) {
>> -        fd = open(
>> -                filename,
>> -                O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> -                0644);
>> +        fd = qemu_open(filename,
>> +                       O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> +                       0644);
>>       } else {
>> -        fd = open(
>> -                filename,
>> -                O_WRONLY | O_BINARY | O_LARGEFILE,
>> -                0644);
>> +        fd = qemu_open(filename,
>> +                       O_WRONLY | O_BINARY | O_LARGEFILE,
>> +                       0644);
>>       }
>>       if (fd < 0) {
>>           return -errno;
>> diff --git a/block/vpc.c b/block/vpc.c
>> index 5cd13d1..60ebf5a 100644
>> --- a/block/vpc.c
>> +++ b/block/vpc.c
>> @@ -678,7 +678,7 @@ static int vpc_create(const char *filename, QEMUOptionParameter *options)
>>       }
>>
>>       /* Create the file */
>> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>> +    fd = qemu_open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY, 0644);
>>       if (fd < 0) {
>>           return -EIO;
>>       }
>> diff --git a/block/vvfat.c b/block/vvfat.c
>> index 0fd3367..81c3a19 100644
>> --- a/block/vvfat.c
>> +++ b/block/vvfat.c
>> @@ -1152,16 +1152,17 @@ static inline mapping_t* find_mapping_for_cluster(BDRVVVFATState* s,int cluster_
>>   static int open_file(BDRVVVFATState* s,mapping_t* mapping)
>>   {
>>       if(!mapping)
>> -	return -1;
>> +        return -1;
>>       if(!s->current_mapping ||
>> -	    strcmp(s->current_mapping->path,mapping->path)) {
>> -	/* open file */
>> -	int fd = open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
>> -	if(fd<0)
>> -	    return -1;
>> -	vvfat_close_current_file(s);
>> -	s->current_fd = fd;
>> -	s->current_mapping = mapping;
>> +            strcmp(s->current_mapping->path, mapping->path)) {
>> +        /* open file */
>> +        int fd = qemu_open(mapping->path, O_RDONLY | O_BINARY | O_LARGEFILE);
>> +        if (fd < 0) {
>> +            return -1;
>> +        }
>> +        vvfat_close_current_file(s);
>> +        s->current_fd = fd;
>> +        s->current_mapping = mapping;
>>       }
>>       return 0;
>>   }
>> @@ -2215,7 +2216,7 @@ static int commit_one_file(BDRVVVFATState* s,
>>       for (i = s->cluster_size; i < offset; i += s->cluster_size)
>>   	c = modified_fat_get(s, c);
>>
>> -    fd = open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
>> +    fd = qemu_open(mapping->path, O_RDWR | O_CREAT | O_BINARY, 0666);
>>       if (fd < 0) {
>>   	fprintf(stderr, "Could not open %s... (%s, %d)\n", mapping->path,
>>   		strerror(errno), errno);
>

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 4/5] block: Convert open calls to qemu_open

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

On 06/14/2012 09:55 AM, Corey Bryant wrote:
> This patch converts all block layer open calls to qemu_open.  This
> enables all block layer open paths to dup(X) a pre-opened file
> descriptor if the filename is of the format /dev/fd/X.  This is
> useful if QEMU is restricted from opening certain files.
> 
> Note that this adds the O_CLOEXEC flag to the changed open paths
> when the O_CLOEXEC macro is defined.
> 

> @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>          if ( bsdPath[ 0 ] != '\0' ) {
>              strcat(bsdPath,"s0");
>              /* some CDs don't have a partition 0 */
> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
> +            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);

Why are we even bothering with O_LARGEFILE?  Shouldn't we be compiling
with large file support always enabled, so that we are always calling
open64 in the cases where it matters, without having to explicitly add
the O_LARGEFILE flag ourselves?


> +++ b/block/vdi.c
> @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>          options++;
>      }
>  
> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
> -              0644);
> +    fd = qemu_open(filename,
> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,

More instances.  In fact, scrubbing O_LARGEFILE, is probably worth a
separate patch.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 4/5] block: Convert open calls to qemu_open

[Corey Bryant]

On 06/15/2012 11:21 AM, Eric Blake wrote:
> On 06/14/2012 09:55 AM, Corey Bryant wrote:
>> This patch converts all block layer open calls to qemu_open.  This
>> enables all block layer open paths to dup(X) a pre-opened file
>> descriptor if the filename is of the format /dev/fd/X.  This is
>> useful if QEMU is restricted from opening certain files.
>>
>> Note that this adds the O_CLOEXEC flag to the changed open paths
>> when the O_CLOEXEC macro is defined.
>>
>
>> @@ -741,7 +741,7 @@ static int hdev_open(BlockDriverState *bs, const char *filename, int flags)
>>           if ( bsdPath[ 0 ] != '\0' ) {
>>               strcat(bsdPath,"s0");
>>               /* some CDs don't have a partition 0 */
>> -            fd = open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>> +            fd = qemu_open(bsdPath, O_RDONLY | O_BINARY | O_LARGEFILE);
>
> Why are we even bothering with O_LARGEFILE?  Shouldn't we be compiling
> with large file support always enabled, so that we are always calling
> open64 in the cases where it matters, without having to explicitly add
> the O_LARGEFILE flag ourselves?
>

That might be a no-op too since large file support looks to already be 
set at compile time:

257 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 
-D_LARGEFILE_SOURCE $QEMU_CFLAGS"

>
>> +++ b/block/vdi.c
>> @@ -648,8 +648,9 @@ static int vdi_create(const char *filename, QEMUOptionParameter *options)
>>           options++;
>>       }
>>
>> -    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>> -              0644);
>> +    fd = qemu_open(filename,
>> +                   O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | O_LARGEFILE,
>
> More instances.  In fact, scrubbing O_LARGEFILE, is probably worth a
> separate patch.
>

I'm happy to do it.  Kevin or anyone else want to weigh in first?

-- 
Regards,
Corey

[Qemu-devel] [PATCH v3 5/5] block: Prevent /dev/fd/X filename from being detected as floppy

[Corey Bryant]

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
---
v3:
 -This patch is new in v3.  It was previously submitted on its
  own, and is now being included in this series.

 block/raw-posix.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index d8eff2f..68886cd 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename)
     int prio = 0;
     struct floppy_struct fdparam;
     struct stat st;
+    const char *p;
 
-    if (strstart(filename, "/dev/fd", NULL))
+    if (strstart(filename, "/dev/fd", &p) && p[0] != '/') {
         prio = 50;
+    }
 
     fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
     if (fd < 0) {
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH v3 5/5] block: Prevent /dev/fd/X filename from being detected as floppy

[Luiz Capitulino]

On Thu, 14 Jun 2012 11:55:05 -0400
Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:

> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>

This too, would be better to move before the /dev/fd/X feature so that we
avoid having introducing a bug and fixing it in a later commit. But again,
it's not a huge issue.

> ---
> v3:
>  -This patch is new in v3.  It was previously submitted on its
>   own, and is now being included in this series.
> 
>  block/raw-posix.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index d8eff2f..68886cd 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename)
>      int prio = 0;
>      struct floppy_struct fdparam;
>      struct stat st;
> +    const char *p;
>  
> -    if (strstart(filename, "/dev/fd", NULL))
> +    if (strstart(filename, "/dev/fd", &p) && p[0] != '/') {
>          prio = 50;
> +    }
>  
>      fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>      if (fd < 0) {

Re: [Qemu-devel] [PATCH v3 5/5] block: Prevent /dev/fd/X filename from being detected as floppy

[Corey Bryant]

On 06/15/2012 10:38 AM, Luiz Capitulino wrote:
> On Thu, 14 Jun 2012 11:55:05 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> Reported-by: Kevin Wolf <kwolf@redhat.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>
> This too, would be better to move before the /dev/fd/X feature so that we
> avoid having introducing a bug and fixing it in a later commit. But again,
> it's not a huge issue.
>

Yes, this one makes even more sense to move earlier in the series.  Thanks.

>> ---
>> v3:
>>   -This patch is new in v3.  It was previously submitted on its
>>    own, and is now being included in this series.
>>
>>   block/raw-posix.c |    4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/raw-posix.c b/block/raw-posix.c
>> index d8eff2f..68886cd 100644
>> --- a/block/raw-posix.c
>> +++ b/block/raw-posix.c
>> @@ -946,9 +946,11 @@ static int floppy_probe_device(const char *filename)
>>       int prio = 0;
>>       struct floppy_struct fdparam;
>>       struct stat st;
>> +    const char *p;
>>
>> -    if (strstart(filename, "/dev/fd", NULL))
>> +    if (strstart(filename, "/dev/fd", &p) && p[0] != '/') {
>>           prio = 50;
>> +    }
>>
>>       fd = qemu_open(filename, O_RDONLY | O_NONBLOCK);
>>       if (fd < 0) {
>

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1356 bytes --]

On 06/14/2012 09:55 AM, Corey Bryant wrote:

> This patch series adds the pass-fd QMP monitor command, which allows
> an fd to be passed via SCM_RIGHTS, and returns the received file
> descriptor.  Support is also added to the block layer to allow QEMU
> to dup the fd when the filename is of the /dev/fd/X format.  This
> is useful if MAC policy prevents QEMU from opening specific types
> of files.

Something to think about on how this will interact with Jeff's work on
block-commit.  That is an interface where qemu must reopen backing files
which were previously O_RDONLY to now be O_RDWR.  By default (when
open() is supported in qemu, and qemu is using the actual file name),
this means 'block-commit' works without needing any fd names.  But with
this new fd-passing approach, a file originally opened as O_RDONLY
/dev/fd/21 will need to be reopened, but the reopened fd will (likely)
not be 21.  In other words, we need to make sure 'block-commit' supports
the ability to pass in optional arguments that specify the file name of
the backing file to be reopened, so that libvirt can pass in O_RDWR fds
to replace the existing O_RDONLY fd, and be aware that the /dev/fd/nn
naming of the reopen will be different.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Kevin Wolf]

Am 19.06.2012 17:46, schrieb Eric Blake:
> On 06/14/2012 09:55 AM, Corey Bryant wrote:
> 
>> This patch series adds the pass-fd QMP monitor command, which allows
>> an fd to be passed via SCM_RIGHTS, and returns the received file
>> descriptor.  Support is also added to the block layer to allow QEMU
>> to dup the fd when the filename is of the /dev/fd/X format.  This
>> is useful if MAC policy prevents QEMU from opening specific types
>> of files.
> 
> Something to think about on how this will interact with Jeff's work on
> block-commit.  That is an interface where qemu must reopen backing files
> which were previously O_RDONLY to now be O_RDWR.  By default (when
> open() is supported in qemu, and qemu is using the actual file name),
> this means 'block-commit' works without needing any fd names.  But with
> this new fd-passing approach, a file originally opened as O_RDONLY
> /dev/fd/21 will need to be reopened, but the reopened fd will (likely)
> not be 21.  In other words, we need to make sure 'block-commit' supports
> the ability to pass in optional arguments that specify the file name of
> the backing file to be reopened, so that libvirt can pass in O_RDWR fds
> to replace the existing O_RDONLY fd, and be aware that the /dev/fd/nn
> naming of the reopen will be different.

Adding an extra argument to each command that reopens (as in
bdrv_reopen(), i.e. changes flags) internally is one option. In my
opinion not a particularly nice one, though.

Maybe it's better to have a monitor command that just prepares a reopen
and means "for the next reopen of /dev/fd/42, the passed FD will have
the right flags (if it hasn't, the reopen will fail)". We can use dup2()
to keep the "name" stable.

Kevin

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]

On 06/19/2012 09:57 AM, Kevin Wolf wrote:

>> this new fd-passing approach, a file originally opened as O_RDONLY
>> /dev/fd/21 will need to be reopened, but the reopened fd will (likely)
>> not be 21.  In other words, we need to make sure 'block-commit' supports
>> the ability to pass in optional arguments that specify the file name of
>> the backing file to be reopened,

> 
> Adding an extra argument to each command that reopens (as in
> bdrv_reopen(), i.e. changes flags) internally is one option. In my
> opinion not a particularly nice one, though.

Agreed, as that's a lot of work for a lot of commands.

> 
> Maybe it's better to have a monitor command that just prepares a reopen
> and means "for the next reopen of /dev/fd/42, the passed FD will have
> the right flags (if it hasn't, the reopen will fail)". We can use dup2()
> to keep the "name" stable.

Indeed, having one additional up-front command in the pass-fd/closefd
family might make this easier.  But how would it work reliably?
Remember, the current proposal is:

libvirt opens backing file O_RDONLY, and calls 'pass-fd name'
qemu returns 21
libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file
qemu dup()s 21, and proceeds to use fd 22 for all its real work
libvirt calls 'closefd name', to avoid the leak on fd 21

sometime later...
qemu has opened something else that now occupies fd 21
libvirt wants to call 'block-commit', and knows qemu now needs O_RDWR
access to the file
under your idea, that would mean libvirt would call something like
'pass-reopen-fd name /dev/fd/21', and qemu would get a new fd (let's
assume 23), so that qemu would now know that fd 23 should be used the
next time any qemu interface wants to reopen '/dev/fd/21'

But that's not safe - there could easily have been more than one
'pass-fd' all resulting in 21, as long as each was separated by
'closefd' in the meantime; and since libvirt already called 'closefd' to
avoid an indefinite fd leak, qemu is tracking neither /dev/fd/21 nor
'name' in its passfd list.  That is, the only place tracking
'/dev/fd/21' is the block driver where we used /dev/fd to pass in the
O_RDONLY fd in the first place.  Qemu can't dup2() to move 23 into 21 at
the time of the 'pass-reopen-fd', as that might not be safe.

Unless you have any bright ideas that I'm overlooking, I don't see how
an additional 'pass-reopen-fd' can be made to do what we want.  I'm
afraid that the only way to make a reopen operation reliable is to be
able to specify a new file name to pass in a new /dev/fd/nnn to use as
part of any reopen operation, which means touching every command like
'block-commit' that needs to do a reopen.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Kevin Wolf]

Am 19.06.2012 18:14, schrieb Eric Blake:
>> Maybe it's better to have a monitor command that just prepares a reopen
>> and means "for the next reopen of /dev/fd/42, the passed FD will have
>> the right flags (if it hasn't, the reopen will fail)". We can use dup2()
>> to keep the "name" stable.
> 
> Indeed, having one additional up-front command in the pass-fd/closefd
> family might make this easier.  But how would it work reliably?
> Remember, the current proposal is:
> 
> libvirt opens backing file O_RDONLY, and calls 'pass-fd name'
> qemu returns 21
> libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file
> qemu dup()s 21, and proceeds to use fd 22 for all its real work
> libvirt calls 'closefd name', to avoid the leak on fd 21

Right, I didn't consider that we really use a dup()ed fd.

I'm not completely clear about when libvirt should call closefd, and
what the lifetime of fd 21 is. If I'm not mistaken, the reason for using
dup() and requiring an explicit closefd was that qemu can reopen the
file multiple times, for example for probing, but it's basically the
same with commit. If so, nothing else must become fd 21 while the image
is still in use as qemu might decide to reopen.

This might mean that libvirt should only closefd the file when it
becomes unused (like after hot unplug); or that qemu must keep it open
internally even after closefd as long as the block device is still in use.

Kevin

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Daniel P. Berrange]

On Wed, Jun 20, 2012 at 09:25:29AM +0200, Kevin Wolf wrote:
> Am 19.06.2012 18:14, schrieb Eric Blake:
> >> Maybe it's better to have a monitor command that just prepares a reopen
> >> and means "for the next reopen of /dev/fd/42, the passed FD will have
> >> the right flags (if it hasn't, the reopen will fail)". We can use dup2()
> >> to keep the "name" stable.
> > 
> > Indeed, having one additional up-front command in the pass-fd/closefd
> > family might make this easier.  But how would it work reliably?
> > Remember, the current proposal is:
> > 
> > libvirt opens backing file O_RDONLY, and calls 'pass-fd name'
> > qemu returns 21
> > libvirt tells qemu to hotplug a drive with /dev/fd/21 as backing file
> > qemu dup()s 21, and proceeds to use fd 22 for all its real work
> > libvirt calls 'closefd name', to avoid the leak on fd 21
> 
> Right, I didn't consider that we really use a dup()ed fd.
> 
> I'm not completely clear about when libvirt should call closefd, and
> what the lifetime of fd 21 is. If I'm not mistaken, the reason for using
> dup() and requiring an explicit closefd was that qemu can reopen the
> file multiple times, for example for probing, but it's basically the
> same with commit. If so, nothing else must become fd 21 while the image
> is still in use as qemu might decide to reopen.
> 
> This might mean that libvirt should only closefd the file when it
> becomes unused (like after hot unplug); or that qemu must keep it open
> internally even after closefd as long as the block device is still in use.

As it works today, the only time libvirt would call "closefd", is if
the monitor command it was trying to use the FD with (eg drive_add)
failed. If drive_add was successfully run, then libvirt would not be
invoking closefd.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1780 bytes --]

On 06/20/2012 02:31 AM, Daniel P. Berrange wrote:

>> This might mean that libvirt should only closefd the file when it
>> becomes unused (like after hot unplug); or that qemu must keep it open
>> internally even after closefd as long as the block device is still in use.
> 
> As it works today, the only time libvirt would call "closefd", is if
> the monitor command it was trying to use the FD with (eg drive_add)
> failed. If drive_add was successfully run, then libvirt would not be
> invoking closefd.

But right now, the only time libvirt uses 'getfd' is with commands like
'migrate' that implicitly close the fd after it is used by name; we
don't have any experience in using fds by '/dev/fd/nn' notation instead
of name.  It is the fact that /dev/fd/nn will allow us to use 'pass-fd'
in more situations in before, by dup()ing the fd, that libvirt all the
sudden becomes responsible for using 'closefd' at the appropriate moments.

I guess I can live with a rule that libvirt must not call 'closefd' on
any fd that it might later want to reassign to a new copy of the fd;
that is, with backing chains, if libvirt originally calls 'pass-fd
drive-virtio1' with an O_RDONLY fd and gets back 21, then libvirt must
not call 'closefd drive-virtio1' until it knows drive-virtio1 is no
longer needed.  Converting the 'drive-virtio1' fd to O_RDWR while still
keeping it at /dev/fd/21 could be done via 'pass-fd -f drive-virtio1',
where -f is an optional bool parameter to force a reassociation of a
given name back to the previously assigned value instead of the normal
error path for accidentally passing an fd to an already in-use name.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Corey Bryant]

On 06/20/2012 07:24 AM, Eric Blake wrote:
> On 06/20/2012 02:31 AM, Daniel P. Berrange wrote:
>
>>> This might mean that libvirt should only closefd the file when it
>>> becomes unused (like after hot unplug); or that qemu must keep it open
>>> internally even after closefd as long as the block device is still in use.
>>
>> As it works today, the only time libvirt would call "closefd", is if
>> the monitor command it was trying to use the FD with (eg drive_add)
>> failed. If drive_add was successfully run, then libvirt would not be
>> invoking closefd.
>
> But right now, the only time libvirt uses 'getfd' is with commands like
> 'migrate' that implicitly close the fd after it is used by name; we
> don't have any experience in using fds by '/dev/fd/nn' notation instead
> of name.  It is the fact that /dev/fd/nn will allow us to use 'pass-fd'
> in more situations in before, by dup()ing the fd, that libvirt all the
> sudden becomes responsible for using 'closefd' at the appropriate moments.
>
> I guess I can live with a rule that libvirt must not call 'closefd' on
> any fd that it might later want to reassign to a new copy of the fd;
> that is, with backing chains, if libvirt originally calls 'pass-fd
> drive-virtio1' with an O_RDONLY fd and gets back 21, then libvirt must
> not call 'closefd drive-virtio1' until it knows drive-virtio1 is no
> longer needed.  Converting the 'drive-virtio1' fd to O_RDWR while still
> keeping it at /dev/fd/21 could be done via 'pass-fd -f drive-virtio1',
> where -f is an optional bool parameter to force a reassociation of a
> given name back to the previously assigned value instead of the normal
> error path for accidentally passing an fd to an already in-use name.
>

It sounds like the flow would be:
'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21
'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21?

But I'm not clear as to how you would retain file descriptor 21 in the 
guest when using 'pass-fd -f drive-virtio1'.  The fd is created as a 
result of passing via SCM_RIGHTS, which assigns the next available fd. 
We don't have control over what fd is assigned, do we?

-- 
Regards,
Corey

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

On 06/20/2012 07:31 AM, Corey Bryant wrote:

> 
> It sounds like the flow would be:
> 'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21

No -f, so qemu errors out if an fd named 'drive-virtio1' already exists;
otherwise it succeeds, and returns the fd assigned by SCM_RIGHTS as well
as adding the name to its internal list.

> 'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21?

-f says to fail if 'drive-virtio1' does not already exist in the
internal list.  Otherwise, this is a reopen attempt, and the fd passed
in by SCM_RIGHTS (let's assume it is 23 at this time) is then passed
through dup2() to overwrite the fd already associated with
'drive-virtio1' (21 in this case), then the SCM_RIGHTS fd (23) is
closed.  In this way, the name 'drive-virtio1' remains associated with
fd 21, but we have reopened it with different mode.  At this point, code
that wants to reopen /dev/fd/21 with a new mode will see the new
permissions on the reassigned fd.  And yes, it means that libvirt would
not be allowed to call 'closefd drive-virtio1' until the block device
for drive-virtio1 is no longer around, whether or not the /dev/fd/nn
reuses the fd as-is or whether it dup()s the fd to something else (say
22) for use by the block device.

> 
> But I'm not clear as to how you would retain file descriptor 21 in the
> guest when using 'pass-fd -f drive-virtio1'.  The fd is created as a
> result of passing via SCM_RIGHTS, which assigns the next available fd.
> We don't have control over what fd is assigned, do we?

The use of -f says that 'pass-fd' uses dup2() to reuse an existing fd.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH v3 0/5] file descriptor passing using pass-fd

[Corey Bryant]

On 06/20/2012 10:53 AM, Eric Blake wrote:
> On 06/20/2012 07:31 AM, Corey Bryant wrote:
>
>>
>> It sounds like the flow would be:
>> 'pass-fd drive-virtio1' of O_RDONLY fd --> guest gets fd 21
>
> No -f, so qemu errors out if an fd named 'drive-virtio1' already exists;
> otherwise it succeeds, and returns the fd assigned by SCM_RIGHTS as well
> as adding the name to its internal list.
>
>> 'pass-fd -f drive-virtio1' of O_WRONLY fd --> guest gets fd 21?
>
> -f says to fail if 'drive-virtio1' does not already exist in the
> internal list.  Otherwise, this is a reopen attempt, and the fd passed
> in by SCM_RIGHTS (let's assume it is 23 at this time) is then passed
> through dup2() to overwrite the fd already associated with
> 'drive-virtio1' (21 in this case), then the SCM_RIGHTS fd (23) is
> closed.  In this way, the name 'drive-virtio1' remains associated with
> fd 21, but we have reopened it with different mode.  At this point, code
> that wants to reopen /dev/fd/21 with a new mode will see the new
> permissions on the reassigned fd.  And yes, it means that libvirt would
> not be allowed to call 'closefd drive-virtio1' until the block device
> for drive-virtio1 is no longer around, whether or not the /dev/fd/nn
> reuses the fd as-is or whether it dup()s the fd to something else (say
> 22) for use by the block device.
>

That makes sense.  Thanks for the explanation.

Essentially dup2(23, 21) will give fd 21 the new access mode.  I assume 
we can go ahead and close fd 23 after the dup2?

I'll go ahead and add this support in the next version of the patch 
series if there are no objections.

-- 
Regards,
Corey

>>
>> But I'm not clear as to how you would retain file descriptor 21 in the
>> guest when using 'pass-fd -f drive-virtio1'.  The fd is created as a
>> result of passing via SCM_RIGHTS, which assigns the next available fd.
>> We don't have control over what fd is assigned, do we?
>
> The use of -f says that 'pass-fd' uses dup2() to reuse an existing fd.
>