Re: [Qemu-devel] [PATCH v2] chardev: Avoid adding duplicate chardev

[Marc-André Lureau]

Hi

On Tue, Jan 29, 2019 at 7:36 AM Pankaj Gupta <address@hidden> wrote:
>
> Hotplugging existing char chardev with qmp, dereferences(removes)
> existing chardev. This patch avoids adding a chardev if a chardev
> with same id exists.

As you pointed out, if you attempt to add a chardev with an existing
ID, you get an error:

{"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket","data":{"addr":{"type":"unix",
"data": {"path": "/tmp/helloworld1"}}}}}}
{"return": {}}
{"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket","data":{"addr":{"type":"unix",
"data": {"path": "/tmp/helloworld1"}}}}}}
{"error": {"class": "GenericError", "desc": "attempt to add duplicate
property 'charchannel1' to object (type 'container')"}}


But the existing chardev is left untouched.

However, since unix socket chardev will delete existing file and
rebind (this is not always a good idea, but people seem to prefer
that)
the rebound socket is removed on error cleanup.


I am not sure this is a bug tbh.

Your solution to check for duplicate ID upfront is ok. But any other
later error path could have the same "bug" effect of removing existing
chardev because of the overwrite socket creation.

Daniel, you may want to comment (we had a similar discussion about
Spice server unix sockets recently)

thanks

>
> RH BZ 1660831:
>
> # (host) ls -lt /tmp/helloworld*
> srwxr-xr-x.  /tmp/helloworld1
> srwxr-xr-x.  /tmp/helloworld2
>
> Before this patch:
>
> hotplug existed chardev(channel1) in qmp:
> {"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket",
> "data":{"addr":{"type":"unix", "data": {"path": "/tmp/helloworld1"}}}}}}
>
> {"error": {"class": "GenericError", "desc": "attempt to add duplicate
> property 'charchannel1' to object (type 'container')"}}
>
> # ls -lt /tmp/helloworld*
> srwxr-xr-x. 1 root root 0 Dec 19 16:39 /tmp/helloworld2
>
> After this patch:
>
> {"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket",
> "data":{"addr":{"type":"unix", "data": {"path": "/tmp/helloworld1"}}}}}}
> {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' already 
> exists"}}
>
> # ls -lt /tmp/helloworld*
> srwxr-xr-x. 1 /tmp/helloworld1
> srwxr-xr-x. 1 /tmp/helloworld2
>
> Reported-by: Xiaohui Li <address@hidden>
> Signed-off-by: Pankaj Gupta <address@hidden>
> ---
>
> v1->v2
>  Correct error message - Eric
>
>  chardev/char.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/chardev/char.c b/chardev/char.c
> index ccba36bafb..cab0d3df16 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -985,6 +985,12 @@ ChardevReturn *qmp_chardev_add(const char *id, 
> ChardevBackend *backend,
>      ChardevReturn *ret;
>      Chardev *chr;
>
> +    chr = qemu_chr_find(id);
> +    if (chr) {
> +        error_setg(errp, "Chardev '%s' already exists", id);
> +        return NULL;
> +    }
> +
>      cc = char_get_class(ChardevBackendKind_str(backend->type), errp);
>      if (!cc) {
>          return NULL;
> --
> 2.14.3
>
>


-- 
Marc-André Lureau