Re: [libvirt] [Qemu-devel] [PATCH v2 2/4] qapi: Add passfd QMP command
On Fri, 8 Jun 2012 11:42:57 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the passfd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the passfd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
Let's call it pass-fd instead. Also, getfd automatically closes a fd if an existing fdname is passed again. I don't think this is a good behavior, I think pass-fd should fail instead (note that we can't fix getfd though).
The closefd command can be used to close a file descriptor that was passed with the passfd command.
v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- monitor.c | 14 ++++++++++---- qapi-schema.json | 15 +++++++++++++++ qmp-commands.hx | 27 +++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c index 4c53cb6..980a226 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,7 +2182,7 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
-void qmp_getfd(const char *fdname, Error **errp) +int64_t qmp_passfd(const char *fdname, Error **errp) { mon_fd_t *monfd; int fd; @@ -2190,13 +2190,13 @@ void qmp_getfd(const char *fdname, Error **errp) fd = qemu_chr_fe_get_msgfd(cur_mon->chr); if (fd == -1) { error_set(errp, QERR_FD_NOT_SUPPLIED); - return; + return -1; }
if (qemu_isdigit(fdname[0])) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", "a name not starting with a digit"); - return; + return -1; }
QLIST_FOREACH(monfd, &cur_mon->fds, next) { @@ -2206,7 +2206,7 @@ void qmp_getfd(const char *fdname, Error **errp)
close(monfd->fd); monfd->fd = fd; - return; + return fd; }
monfd = g_malloc0(sizeof(mon_fd_t)); @@ -2214,6 +2214,12 @@ void qmp_getfd(const char *fdname, Error **errp) monfd->fd = fd;
QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + +void qmp_getfd(const char *fdname, Error **errp) +{ + qmp_passfd(fdname, errp); return; }
diff --git a/qapi-schema.json b/qapi-schema.json index 6be1d90..15da1b8 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,21 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @passfd: +# +# 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 +## +{ 'command': 'passfd', '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 f8c0f68..338a0b3 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,33 @@ Example: EQMP
{ + .name = "passfd", + .args_type = "fdname:s", + .params = "passfd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_passfd, + }, + +SQMP +passfd +------ + +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": "passfd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
On 06/13/2012 03:46 PM, Luiz Capitulino wrote:
On Fri, 8 Jun 2012 11:42:57 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
This patch adds the passfd QMP command using the QAPI framework. Like the getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the passfd command also returns the received file descriptor, which is a difference in behavior from the getfd command, which returns nothing.
Let's call it pass-fd instead.
Sure, that works for me.
Also, getfd automatically closes a fd if an existing fdname is passed again. I don't think this is a good behavior, I think pass-fd should fail instead (note that we can't fix getfd though).
I agree. It makes sense to fail rather than blindly closing the existing fd. It can be closed explicitly with closefd if the user wants it closed.
The closefd command can be used to close a file descriptor that was passed with the passfd command.
v2: -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com) -Use passfd as command name (berrange@redhat.com)
Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com> --- monitor.c | 14 ++++++++++---- qapi-schema.json | 15 +++++++++++++++ qmp-commands.hx | 27 +++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-)
diff --git a/monitor.c b/monitor.c index 4c53cb6..980a226 100644 --- a/monitor.c +++ b/monitor.c @@ -2182,7 +2182,7 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif
-void qmp_getfd(const char *fdname, Error **errp) +int64_t qmp_passfd(const char *fdname, Error **errp) { mon_fd_t *monfd; int fd; @@ -2190,13 +2190,13 @@ void qmp_getfd(const char *fdname, Error **errp) fd = qemu_chr_fe_get_msgfd(cur_mon->chr); if (fd == -1) { error_set(errp, QERR_FD_NOT_SUPPLIED); - return; + return -1; }
if (qemu_isdigit(fdname[0])) { error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname", "a name not starting with a digit"); - return; + return -1; }
QLIST_FOREACH(monfd, &cur_mon->fds, next) { @@ -2206,7 +2206,7 @@ void qmp_getfd(const char *fdname, Error **errp)
close(monfd->fd); monfd->fd = fd; - return; + return fd; }
monfd = g_malloc0(sizeof(mon_fd_t)); @@ -2214,6 +2214,12 @@ void qmp_getfd(const char *fdname, Error **errp) monfd->fd = fd;
QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + return fd; +} + +void qmp_getfd(const char *fdname, Error **errp) +{ + qmp_passfd(fdname, errp); return; }
diff --git a/qapi-schema.json b/qapi-schema.json index 6be1d90..15da1b8 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1864,6 +1864,21 @@ { 'command': 'netdev_del', 'data': {'id': 'str'} }
## +# @passfd: +# +# 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 +## +{ 'command': 'passfd', '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 f8c0f68..338a0b3 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -869,6 +869,33 @@ Example: EQMP
{ + .name = "passfd", + .args_type = "fdname:s", + .params = "passfd name", + .help = "pass a file descriptor via SCM rights and assign it a name", + .mhandler.cmd_new = qmp_marshal_input_passfd, + }, + +SQMP +passfd +------ + +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": "passfd", "arguments": { "fdname": "fd1" } } +<- { "return": 42 } + +EQMP + + { .name = "getfd", .args_type = "fdname:s", .params = "getfd name",
-- Regards, Corey
On 06/13/2012 02:25 PM, Corey Bryant wrote:
Also, getfd automatically closes a fd if an existing fdname is passed again. I don't think this is a good behavior, I think pass-fd should fail instead (note that we can't fix getfd though).
I agree. It makes sense to fail rather than blindly closing the existing fd. It can be closed explicitly with closefd if the user wants it closed.
Hmm - what happens if I do 'pass-fd name', learn that qemu is using fd 42, then do 'getfd name'? I silently wipe out fd 42 and replace it with the new fd passed in by getfd. Which means my use of /dev/fd/42 will now be broken. Obviously that means that 'getfd' should NOT be used by any application using 'pass-fd', and that libvirt should NOT be reusing names (I think the latter is already true). But I agree that for back-compat we can't get rid of the current (evil) semantics of a duplicated 'getfd'. You may also want to mention that when using 'getfd' or 'pass-fd', there are some commands (like migrate) that use the fd:name protocol, and that a successful use of one of these commands implicitly closes the named fd; but that all new uses of /dev/fd/nnn leave the fd open and an explicit closefd must be used to avoid leaking indefinitely-opened fds in qemu. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 06/13/2012 04:47 PM, Eric Blake wrote:
On 06/13/2012 02:25 PM, Corey Bryant wrote:
Also, getfd automatically closes a fd if an existing fdname is passed again. I don't think this is a good behavior, I think pass-fd should fail instead (note that we can't fix getfd though).
I agree. It makes sense to fail rather than blindly closing the existing fd. It can be closed explicitly with closefd if the user wants it closed.
Hmm - what happens if I do 'pass-fd name', learn that qemu is using fd 42, then do 'getfd name'? I silently wipe out fd 42 and replace it with the new fd passed in by getfd. Which means my use of /dev/fd/42 will now be broken.
Obviously that means that 'getfd' should NOT be used by any application using 'pass-fd', and that libvirt should NOT be reusing names (I think the latter is already true). But I agree that for back-compat we can't get rid of the current (evil) semantics of a duplicated 'getfd'.
Yes, users need to be careful and understand how the commands work. I don't think it's a hard rule that 'getfd' can't be used by an application that uses 'pass-fd'. If it were, we could put the fds on separate lists: struct Monitor { ... QLIST_HEAD(,mon_fd_t) fds; + QLIST_HEAD(,mon_fd_t) pass_fds; }; But I don't think this is necessary, so I'll plan on documenting them well.
You may also want to mention that when using 'getfd' or 'pass-fd', there are some commands (like migrate) that use the fd:name protocol, and that a successful use of one of these commands implicitly closes the named fd; but that all new uses of /dev/fd/nnn leave the fd open and an explicit closefd must be used to avoid leaking indefinitely-opened fds in qemu.
Ok, I'll mention this too. Thanks. -- Regards, Corey
On Wed, 13 Jun 2012 18:07:43 -0400 Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
On 06/13/2012 04:47 PM, Eric Blake wrote:
On 06/13/2012 02:25 PM, Corey Bryant wrote:
Also, getfd automatically closes a fd if an existing fdname is passed again. I don't think this is a good behavior, I think pass-fd should fail instead (note that we can't fix getfd though).
I agree. It makes sense to fail rather than blindly closing the existing fd. It can be closed explicitly with closefd if the user wants it closed.
Hmm - what happens if I do 'pass-fd name', learn that qemu is using fd 42, then do 'getfd name'? I silently wipe out fd 42 and replace it with the new fd passed in by getfd. Which means my use of /dev/fd/42 will now be broken.
Obviously that means that 'getfd' should NOT be used by any application using 'pass-fd', and that libvirt should NOT be reusing names (I think the latter is already true). But I agree that for back-compat we can't get rid of the current (evil) semantics of a duplicated 'getfd'.
Yes, users need to be careful and understand how the commands work. I don't think it's a hard rule that 'getfd' can't be used by an application that uses 'pass-fd'. If it were, we could put the fds on separate lists:
struct Monitor { ... QLIST_HEAD(,mon_fd_t) fds; + QLIST_HEAD(,mon_fd_t) pass_fds; };
We'd a different closefd command if we do this.
But I don't think this is necessary, so I'll plan on documenting them well.
Agreed, I don't think this is necessary.
participants (3)
-
Corey Bryant -
Eric Blake -
Luiz Capitulino