Re: [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command

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

On 06/15/2012 11:14 AM, Luiz Capitulino wrote:
> On Fri, 15 Jun 2012 11:04:16 -0400
> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>
>>
>>
>> On 06/15/2012 10:32 AM, Luiz Capitulino wrote:
>>> On Thu, 14 Jun 2012 11:55:02 -0400
>>> Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
>>>
>>>> This patch adds the pass-fd QMP command using the QAPI framework.
>>>> Like the getfd command, it is used to pass a file descriptor via
>>>> SCM_RIGHTS.  However, the pass-fd command also returns the received
>>>> file descriptor, which is a difference in behavior from the getfd
>>>> command, which returns nothing.
>>>>
>>>> The closefd command can be used to close a file descriptor that was
>>>> passed with the pass-fd command.
>>>>
>>>> Note that when using getfd or pass-fd, there are some commands
>>>> (e.g. migrate with fd:name) that implicitly close the named fd.
>>>> When this is not the case, closefd must be used to avoid fd leaks.
>>>>
>>>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>>>> ---
>>>> v2:
>>>>    -Introduce new QMP command to pass/return fd (lcapitulino@redhat.com)
>>>>    -Use passfd as command name (berrange@redhat.com)
>>>>
>>>> v3:
>>>>    -Use pass-fd as command name (lcapitulino@redhat.com)
>>>>    -Fail pass-fd if fdname already exists (lcapitulino@redhat.com)
>>>>    -Add notes to QMP command describing behavior in more detail
>>>>     (lcapitulino@redhat.com, eblake@redhat.com)
>>>>    -Add note about fd leakage (eblake@redhat.com)
>>>>
>>>>    monitor.c        |   33 +++++++++++++++++++++++++++++++++
>>>>    qapi-schema.json |   19 +++++++++++++++++++
>>>>    qmp-commands.hx  |   34 ++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 86 insertions(+)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index 1a7f7e7..6d99368 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -2182,6 +2182,39 @@ static void do_inject_mce(Monitor *mon, const QDict *qdict)
>>>>    }
>>>>    #endif
>>>>
>>>> +int64_t qmp_pass_fd(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) {
>>>> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdname",
>>>> +                      "a name that does not already exist");
>>>> +            return -1;
>>>> +        }
>>>> +    }
>>>
>>> Returning the same error class for two different errors is not a good idea.
>>> I think you have two options here. You could return QERR_INVALID_PARAMETER
>>> for the "already exists" case or introduce QERR_FD_EXISTS. The later is
>>> certainly nicer, but we were trying to avoid having too specific errors...
>>>
>>
>> I'm not clear on what the problem is with returning the same error class
>> for two different errors.  Could you explain?  I don't have a problem
>> changing it if it's an issue.
>
> Because an mngt app/user won't know if what has happened was that the fd name
> is invalid or if the fd name already exists.
>

I agree, it makes sense so the management app can program based on the 
error class.  It just seems like the error classes in general should be 
able to be used multiple times if it fits, especially if we're trying to 
avoid having too specific errors.

Maybe (in the future) something like a sub-class could be used? In other 
words to allow using the same error class with unique sub-classes.

I'll plan on updating to use a different error class for this.

>>
>>>> +
>>>> +    monfd = g_malloc0(sizeof(mon_fd_t));
>>>> +    monfd->name = g_strdup(fdname);
>>>> +    monfd->fd = fd;
>>>
>>> Maybe you could try to move this to a separate function to share code with
>>> qmp_getfd()?
>>>
>>
>> Sure, no problem.  I can do that.
>>
>>>> +
>>>> +    QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next);
>>>> +    return fd;
>>>> +}
>>>> +
>>>>    void qmp_getfd(const char *fdname, Error **errp)
>>>>    {
>>>>        mon_fd_t *monfd;
>>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>>> index 26a6b84..ed99f23 100644
>>>> --- a/qapi-schema.json
>>>> +++ b/qapi-schema.json
>>>> @@ -1864,6 +1864,25 @@
>>>>    { 'command': 'netdev_del', 'data': {'id': 'str'} }
>>>>
>>>>    ##
>>>> +# @pass-fd:
>>>> +#
>>>> +# 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
>>>> +#
>>>> +# Notes: If @fdname already exists, the command will fail.
>>>> +#        The 'closefd' command can be used to explicitly close the
>>>> +#        file descriptor when it is no longer needed.
>>>> +##
>>>> +{ 'command': 'pass-fd', '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 e3cf3c5..c039947 100644
>>>> --- a/qmp-commands.hx
>>>> +++ b/qmp-commands.hx
>>>> @@ -869,6 +869,40 @@ Example:
>>>>    EQMP
>>>>
>>>>        {
>>>> +        .name       = "pass-fd",
>>>> +        .args_type  = "fdname:s",
>>>> +        .params     = "pass-fd name",
>>>> +        .help       = "pass a file descriptor via SCM rights and assign it a name",
>>>> +        .mhandler.cmd_new = qmp_marshal_input_pass_fd,
>>>> +    },
>>>> +
>>>> +SQMP
>>>> +pass-fd
>>>> +-------
>>>> +
>>>> +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": "pass-fd", "arguments": { "fdname": "fd1" } }
>>>> +<- { "return": 42 }
>>>> +
>>>> +Notes:
>>>> +
>>>> +(1) If the name specified by the "fdname" argument already exists,
>>>> +    the command will fail.
>>>> +(2) The 'closefd' command can be used to explicitly close the file
>>>> +    descriptor when it is no longer needed.
>>>> +
>>>> +EQMP
>>>> +
>>>> +    {
>>>>            .name       = "getfd",
>>>>            .args_type  = "fdname:s",
>>>>            .params     = "getfd name",
>>>
>>>
>>
>

-- 
Regards,
Corey