Re: [Qemu-devel] [PATCH v2 04/10] chardev: add qmp hotplug commands, with null chardev support

[Eric Blake <eblake@redhat.com>]

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

On 01/10/2013 07:23 AM, Gerd Hoffmann wrote:
> Add chardev-add and chardev-remove qmp commands.  Hotplugging
> a null chardev is supported for now, more will be added later.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---

> +
> +##
> +# @ChardevReturn:
> +#
> +# Return infos about the chardev backend just created.

s/infos/info/ or maybe s/infos/details/


> +ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend,
> +                               Error **errp)
> +{
> +    ChardevReturn *ret = g_new0(ChardevReturn, 1);
> +    CharDriverState *chr = NULL;
> +

Should you add:

    chr = qemu_chr_find(id);
    if (chr) {
        error_setg(errp, "Chardev '%s' already exists", id);
        return NULL;
    }

> +    switch (backend->kind) {
> +    case CHARDEV_BACKEND_KIND_NULL:
> +        chr = qemu_chr_open_null(NULL);
> +        break;
> +    default:
> +        error_setg(errp, "unknown chardev backend (%d)", backend->kind);
> +        break;
> +    }
> +
> +    if (chr == NULL && !error_is_set(errp)) {
> +        error_setg(errp, "Failed to create chardev");
> +    }
> +    if (chr) {
> +        chr->label = g_strdup(id);
> +        chr->avail_connections = 1;
> +        QTAILQ_INSERT_TAIL(&chardevs, chr, next);
> +    }
> +    return ret;

Do you really want to be returning a successful object even when the
error is set, or should you start with ret = NULL, and only do the
g_new0(ChardevReturn, 1) on success?

-- 
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: 619 bytes --]