|
| From: | Corey Bryant |
| Subject: | Re: [Qemu-devel] [PATCH 1/3] qmp/hmp: Add QMP getfd command that returns fd |
| Date: | Wed, 06 Jun 2012 10:04:23 -0400 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:12.0) Gecko/20120430 Thunderbird/12.0.1 |
On 06/05/2012 02:30 PM, Luiz Capitulino wrote:
On Mon, 4 Jun 2012 09:10:08 -0400 Corey Bryant<address@hidden> wrote:This patch adds QMP support for the getfd command using the QAPI framework. Like the HMP getfd command, it is used to pass a file descriptor via SCM_RIGHTS. However, the QMP getfd command also returns the received file descriptor, which is a difference in behavior from the HMP getfd command, which returns nothing.I have a few comments regarding the qapi conversion below, but something important to discuss is that returning an int the way you're doing it is certainly incompatible.
Thanks for your feedback.
Today, we return a dict on success:
{ "return": {} }
But this patch changes it to:
{ "return": 42 }
There are two ways to do this without breaking compatibility:
1. Add a new command (say get-file-descriptor)
What do you think about using getfd2 for the command name? I'm thinking getfd2 may be more obvious that it corresponds to closefd. That assumes we'll use the same array internally to store fds and closefd can be used to close the fd opened by get-file-descriptor/getfd2.
I assume this approach would still return an int: { "return": 42 }
2. Return a type instead, like:
{ "return": { "file-descriptor": 42 } }
I think I prefer item 1, as we could also take the opportunity to fix the
argument type and improve its name. Besides, we don't have a schema to do 2.
Is it fdname that you think could be improved? fdname seems pretty straight forward to me.
Also, please split the getfd conversion to the qapi from changing its return type in two patches (even if we go for a new command, please do convert getfd too, for completeness); and also convert closefd, please.
Ok
More comments below.Signed-off-by: Corey Bryant<address@hidden> --- hmp-commands.hx | 2 +- monitor.c | 37 ++++++++++++++++++++++++++++++++++++- qapi-schema.json | 13 +++++++++++++ qmp-commands.hx | 6 ++++-- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/hmp-commands.hx b/hmp-commands.hx index 18cb415..dfab369 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1211,7 +1211,7 @@ ETEXI .params = "getfd name", .help = "receive a file descriptor via SCM rights and assign it a name", .user_print = monitor_user_noop,The .user_print line should be dropped.
Ok
- .mhandler.cmd_new = do_getfd, + .mhandler.cmd_new = hmp_getfd,You should use 'cmd' (instead of 'cmd_new'). Please, look for other qapi conversions for examples (like a15fef21c).
Ok
}, STEXI diff --git a/monitor.c b/monitor.c index 12a6fe2..6acf5a3 100644 --- a/monitor.c +++ b/monitor.c @@ -2199,7 +2199,7 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict) } #endif -static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data) +static int hmp_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data)This should be moved to hmp.c and the function signature should be different, looking at other qapi conversions will help you.
Ok
{ const char *fdname = qdict_get_str(qdict, "fdname"); mon_fd_t *monfd; @@ -2235,6 +2235,41 @@ static int do_getfd(Monitor *mon, const QDict *qdict, QObject **ret_data) return 0; } +int64_t qmp_getfd(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) { + continue; + } + + close(monfd->fd); + monfd->fd = fd; + return fd; + } + + 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; +} + static int do_closefd(Monitor *mon, const QDict *qdict, QObject **ret_data) { const char *fdname = qdict_get_str(qdict, "fdname"); diff --git a/qapi-schema.json b/qapi-schema.json index 2ca7195..5f26ba2 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -1755,3 +1755,16 @@ # Since: 0.14.0 ## { 'command': 'device_del', 'data': {'id': 'str'} } + +## +# @getfd: +# +# Receive a file descriptor via SCM rights and assign it a name +# +# @fdname: file descriptor name +# +# Returns: The QEMU @fd that was received +# +# Since: 1.2This command exists since 0.14 if I'm not mistaken, only the return type is new.
Ok
+## +{ 'command': 'getfd', 'data': {'fdname': 'str'}, 'returns': 'int' } diff --git a/qmp-commands.hx b/qmp-commands.hx index db980fa..e13d583 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -844,7 +844,7 @@ EQMP .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 @@ -857,10 +857,12 @@ Arguments: - "fdname": file descriptor name (json-string) +Return a json-int with the QEMU file descriptor that was received. + Example: -> { "execute": "getfd", "arguments": { "fdname": "fd1" } } -<- { "return": {} } +<- { "return": 42 } EQMP
-- Regards, Corey
| [Prev in Thread] | Current Thread | [Next in Thread] |