[libvirt] [Qemu-devel] [PATCH v3 2/5] qapi: Add pass-fd QMP command
Corey Bryant
coreyb at linux.vnet.ibm.com
Fri Jun 15 15:29:03 UTC 2012
On 06/15/2012 11:14 AM, Luiz Capitulino wrote:
> On Fri, 15 Jun 2012 11:04:16 -0400
> Corey Bryant <coreyb at 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 at 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 at linux.vnet.ibm.com>
>>>> ---
>>>> v2:
>>>> -Introduce new QMP command to pass/return fd (lcapitulino at redhat.com)
>>>> -Use passfd as command name (berrange at redhat.com)
>>>>
>>>> v3:
>>>> -Use pass-fd as command name (lcapitulino at redhat.com)
>>>> -Fail pass-fd if fdname already exists (lcapitulino at redhat.com)
>>>> -Add notes to QMP command describing behavior in more detail
>>>> (lcapitulino at redhat.com, eblake at redhat.com)
>>>> -Add note about fd leakage (eblake at 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
More information about the libvir-list
mailing list