Re: [Qemu-devel] [PATCH v2 1/4] qapi: Convert getfd and closefd

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

On 06/13/2012 03:41 PM, Luiz Capitulino wrote:
> On Fri,  8 Jun 2012 11:42:56 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>> 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)
>>
>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx  |    6 ++----
>>   hmp.c            |   18 ++++++++++++++++++
>>   hmp.h            |    2 ++
>>   monitor.c        |   34 ++++++++++++++++------------------
>>   qapi-schema.json |   29 +++++++++++++++++++++++++++++
>>   qmp-commands.hx  |    6 ++----
>>   6 files changed, 69 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..4c53cb6 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -2182,48 +2182,46 @@ 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);
>> +    return;
>
> You have a few of this kind of gratuitous return, please drop 'em.
>

Thanks for reviewing.

I'll get a v3 patch series going and remove the unnecessary returns in it.

>>   }
>>
>> -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 +2230,11 @@ 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);
>> +    return;
>>   }
>>
>>   static void do_loadvm(Monitor *mon, const QDict *qdict)
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 3b6e346..6be1d90 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1862,3 +1862,32 @@
>>   # 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
>
> Please, add a note explaining that getfd automatically closes fds if
> fdname matches an existing name.

Ok

>
> Otherwise this conversion looks very good to me.

Good to hear!

>
>> +#
>> +# Since: 0.14.0
>> +##
>> +{ '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..f8c0f68 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
>> @@ -899,8 +898,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
>


-- 
Regards,
Corey