From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SbyWj-0003LI-KM for qemu-devel@nongnu.org; Tue, 05 Jun 2012 14:30:51 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SbyWg-0000J6-4h for qemu-devel@nongnu.org; Tue, 05 Jun 2012 14:30:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SbyWf-0000Ik-HN for qemu-devel@nongnu.org; Tue, 05 Jun 2012 14:30:41 -0400 Date: Tue, 5 Jun 2012 15:30:58 -0300 From: Luiz Capitulino Message-ID: <20120605153058.01083bfd@doriath.home> In-Reply-To: <1338815410-24890-2-git-send-email-coreyb@linux.vnet.ibm.com> References: <1338815410-24890-1-git-send-email-coreyb@linux.vnet.ibm.com> <1338815410-24890-2-git-send-email-coreyb@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/3] qmp/hmp: Add QMP getfd command that returns fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Corey Bryant Cc: kwolf@redhat.com, aliguori@us.ibm.com, eblake@redhat.com, qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com On Mon, 4 Jun 2012 09:10:08 -0400 Corey Bryant 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. 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) 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. 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. More comments below. > Signed-off-by: Corey Bryant > --- > 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. > - .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). > }, > > 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. > { > 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.2 This command exists since 0.14 if I'm not mistaken, only the return type is new. > +## > +{ '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 >