Re: [Qemu-devel] [PATCH v9 2/7] qapi: Introduce add-fd, remove-fd, query-fdsets

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]

On 08/11/2012 07:14 AM, Corey Bryant wrote:
> This patch adds support that enables passing of file descriptors
> to the QEMU monitor where they will be stored in specified file
> descriptor sets.
> 

> 
> v9:
>  -Use fdset-id rather than fdset_id. (eblake@redhat.com)
>  -Update example for query-fdsets. (eblake@redhat.com)
>  -Close fd immediately on remove-fd.
>   (kwolf@redhat.com, eblake@redhat.com)
>  -Drop fdset refcount, and check dup_fds instead (in a later patch).
>   (eblake@redhat.com)
>  -Move mon_refcount code to a later patch. (kwolf@redhat.com)
> 

> +AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque,
> +                      const char *opaque, Error **errp)
> +{
> +    int fd;
> +    Monitor *mon = cur_mon;
> +    MonFdset *mon_fdset;
> +    MonFdsetFd *mon_fdset_fd;
> +    AddfdInfo *fdinfo;
> +
> +    fd = qemu_chr_fe_get_msgfd(mon->chr);
> +    if (fd == -1) {
> +        error_set(errp, QERR_FD_NOT_SUPPLIED);
> +        goto error;
> +    }
> +
> +    if (has_fdset_id) {
> +        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> +            if (mon_fdset->id == fdset_id) {
> +                break;
> +            }
> +        }
> +        if (mon_fdset == NULL) {
> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
> +                      "an existing fdset-id or no fdset-id");

The 'no fdset-id' portion of this error message doesn't make sense - it
can only be reached if has_fdset_id was true.

> +
> +void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp)
> +{
> +    MonFdset *mon_fdset;
> +    MonFdsetFd *mon_fdset_fd;
> +    char fd_str[60];
> +
> +    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
...

> +    }
> +
> +error:
> +    snprintf(fd_str, sizeof(fd_str),
> +             "fdset-id:%" PRId64 ", fd:%" PRId64, fdset_id, fd);

Oops - fd is uninitialized if has_fd is false and the outer loop failed
to find fdset_id.  You need two separate error messages here, based on
whether fd was provided.

> +-> { "execute": "query-fdsets" }
> +<- { "return": [
> +       {
> +         "fds": [
> +           {
> +             "fd": 30,
> +             "opaque": "rdonly:/path/to/file"
> +           },
> +           {
> +             "fd": 24,
> +             "opaque": "rdwr:/path/to/file"
> +           }
> +         ],
> +         "fdset-id": 1
> +       },
> +       {
> +         "fds": [
> +           {
> +             "fd": 28
> +           },
> +           {
> +             "fd": 29
> +           }
> +         ],
> +         "fdset-id": 0
> +       },

No trailing comma here.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]