Re: [Qemu-devel] [PATCH 1/3] qmp/hmp: Add QMP getfd command that returns fd

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 06/05/2012 02:30 PM, Luiz Capitulino wrote:
> On Mon,  4 Jun 2012 09:10:08 -0400
> Corey Bryant<coreyb@linux.vnet.ibm.com>  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<coreyb@linux.vnet.ibm.com>
>> ---
>>   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.2
>
> This 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