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

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

I'll send a new version shortly with these updates.

-- 
Regards,
Corey

On 08/11/2012 10:16 AM, Eric Blake wrote:
> 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.
>