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

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

[Pankaj Gupta]

Hotplugging existing char chardev with qmp, dereferences(removes) 
existing chardev. This patch avoids adding a chardev if a chardev 
with same id exists.

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 <xiaohli@redhat.com>
Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
---

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

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

[Stefano Garzarella]

On Tue, Jan 29, 2019 at 11:58:01AM +0530, Pankaj Gupta wrote:
> Hotplugging existing char chardev with qmp, dereferences(removes) 
> existing chardev. This patch avoids adding a chardev if a chardev 
> with same id exists.
> 
> 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 <xiaohli@redhat.com>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
> 
> v1->v2
>  Correct error message - Eric 
> 
>  chardev/char.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

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 <pagupta@redhat.com> 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 <xiaohli@redhat.com>
> Signed-off-by: Pankaj Gupta <pagupta@redhat.com>
> ---
>
> 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

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

[Daniel P. Berrangé]

On Wed, Feb 06, 2019 at 05:08:25PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 29, 2019 at 7:36 AM Pankaj Gupta <pagupta@redhat.com> 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.

Checking the ID is not a useful fix IMHO. Someone could just as easily
send 2 commands with different IDs and the same socket path.

A more accurate fix would be to iterate over existing chardevs and check
whether any of them clash, but even that is useless if you have two
separate QEMU instances and both try to use the same UNIX socket path.
To deal with that you need to start taking out fcntl locks to ensure
real mutual exclusion.

I think I'd really just call this user error and do nothing

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Pankaj Gupta]

Hi Daniel, Marc-Andre,

Thanks for your reply. Please find my reply inline.

> > > 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.
> 
> Checking the ID is not a useful fix IMHO. Someone could just as easily
> send 2 commands with different IDs and the same socket path.
> 
> A more accurate fix would be to iterate over existing chardevs and check
> whether any of them clash, but even that is useless if you have two
> separate QEMU instances and both try to use the same UNIX socket path.
> To deal with that you need to start taking out fcntl locks to ensure
> real mutual exclusion.

The reason we are already throwing error "attempt to add duplicate property"
implies we are considering "id" as primary key? Even if we throw the error
existing chardev should work as before. But this is not the case right now, 
it just deletes the existing chardev after error.  

Thanks,
Pankaj

> 
> I think I'd really just call this user error and do nothing
> 
> Regards,
> Daniel
> --
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange
> |:|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com
> |:|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange
> |:|
> 

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

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 8:21 AM Pankaj Gupta <pagupta@redhat.com> wrote:
>
>
> Hi Daniel, Marc-Andre,
>
> Thanks for your reply. Please find my reply inline.
>
> > > > 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.
> >
> > Checking the ID is not a useful fix IMHO. Someone could just as easily
> > send 2 commands with different IDs and the same socket path.
> >
> > A more accurate fix would be to iterate over existing chardevs and check
> > whether any of them clash, but even that is useless if you have two
> > separate QEMU instances and both try to use the same UNIX socket path.
> > To deal with that you need to start taking out fcntl locks to ensure
> > real mutual exclusion.
>
> The reason we are already throwing error "attempt to add duplicate property"
> implies we are considering "id" as primary key? Even if we throw the error
> existing chardev should work as before. But this is not the case right now,
> it just deletes the existing chardev after error.

It deletes the socket "file" (since it overwrites it on chardev
creation). The existing chardev is not deleted:

qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 3},
"package": "qemu-3.0.0-3.fc29"}, "capabilities": []}}
{"execute":"qmp_capabilities"}
{"return": {}}
 {"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')"}}
{"execute":"query-chardev"}
{"return": [{"frontend-open": true, "filename": "vc", "label":
"serial0"}, {"frontend-open": true, "filename": "stdio", "label":
"compat_monitor0"}, {"frontend-open": false, "filename":
"disconnected:unix:/tmp/helloworld1,server", "label": "charchannel1"},
{"frontend-open": true, "filename": "vc", "label": "parallel0"}]}


>
> Thanks,
> Pankaj
>
> >
> > I think I'd really just call this user error and do nothing
> >
> > Regards,
> > Daniel
> > --
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange
> > |:|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com
> > |:|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange
> > |:|
> >



-- 
Marc-André Lureau

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

[Daniel P. Berrangé]

On Thu, Feb 07, 2019 at 10:11:29AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Feb 7, 2019 at 8:21 AM Pankaj Gupta <pagupta@redhat.com> wrote:
> >
> >
> > Hi Daniel, Marc-Andre,
> >
> > Thanks for your reply. Please find my reply inline.
> >
> > > > > 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.
> > >
> > > Checking the ID is not a useful fix IMHO. Someone could just as easily
> > > send 2 commands with different IDs and the same socket path.
> > >
> > > A more accurate fix would be to iterate over existing chardevs and check
> > > whether any of them clash, but even that is useless if you have two
> > > separate QEMU instances and both try to use the same UNIX socket path.
> > > To deal with that you need to start taking out fcntl locks to ensure
> > > real mutual exclusion.
> >
> > The reason we are already throwing error "attempt to add duplicate property"
> > implies we are considering "id" as primary key? Even if we throw the error
> > existing chardev should work as before. But this is not the case right now,
> > it just deletes the existing chardev after error.
> 
> It deletes the socket "file" (since it overwrites it on chardev
> creation). The existing chardev is not deleted:

I think this is yet another example of why it is a bad idea for the
qemu_chardev_new API to also open the backend. We should have a
qemu_chardev_new that only does the arg parsing, object creation
and registration. Then have a separate API for actually opening
it.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Pankaj Gupta]

> > > Thanks for your reply. Please find my reply inline.
> > >
> > > > > > 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.
> > > >
> > > > Checking the ID is not a useful fix IMHO. Someone could just as easily
> > > > send 2 commands with different IDs and the same socket path.
> > > >
> > > > A more accurate fix would be to iterate over existing chardevs and
> > > > check
> > > > whether any of them clash, but even that is useless if you have two
> > > > separate QEMU instances and both try to use the same UNIX socket path.
> > > > To deal with that you need to start taking out fcntl locks to ensure
> > > > real mutual exclusion.
> > >
> > > The reason we are already throwing error "attempt to add duplicate
> > > property"
> > > implies we are considering "id" as primary key? Even if we throw the
> > > error
> > > existing chardev should work as before. But this is not the case right
> > > now,
> > > it just deletes the existing chardev after error.
> > 
> > It deletes the socket "file" (since it overwrites it on chardev
> > creation). The existing chardev is not deleted:
> 
> I think this is yet another example of why it is a bad idea for the
> qemu_chardev_new API to also open the backend. We should have a
> qemu_chardev_new that only does the arg parsing, object creation
> and registration. Then have a separate API for actually opening
> it.

Agree. This looks bigger fix. Till we fix that. Can we accept this patch
to avoid deleting/corrupting existing socket "file" if user tries to add 
chardev with existing same "id"?

Thanks,
Pankaj

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

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 11:13 AM Pankaj Gupta <pagupta@redhat.com> wrote:
>
>
> > > > Thanks for your reply. Please find my reply inline.
> > > >
> > > > > > > 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.
> > > > >
> > > > > Checking the ID is not a useful fix IMHO. Someone could just as easily
> > > > > send 2 commands with different IDs and the same socket path.
> > > > >
> > > > > A more accurate fix would be to iterate over existing chardevs and
> > > > > check
> > > > > whether any of them clash, but even that is useless if you have two
> > > > > separate QEMU instances and both try to use the same UNIX socket path.
> > > > > To deal with that you need to start taking out fcntl locks to ensure
> > > > > real mutual exclusion.
> > > >
> > > > The reason we are already throwing error "attempt to add duplicate
> > > > property"
> > > > implies we are considering "id" as primary key? Even if we throw the
> > > > error
> > > > existing chardev should work as before. But this is not the case right
> > > > now,
> > > > it just deletes the existing chardev after error.
> > >
> > > It deletes the socket "file" (since it overwrites it on chardev
> > > creation). The existing chardev is not deleted:
> >
> > I think this is yet another example of why it is a bad idea for the
> > qemu_chardev_new API to also open the backend. We should have a
> > qemu_chardev_new that only does the arg parsing, object creation
> > and registration. Then have a separate API for actually opening
> > it.
>
> Agree. This looks bigger fix. Till we fix that. Can we accept this patch
> to avoid deleting/corrupting existing socket "file" if user tries to add
> chardev with existing same "id"?

It's not a proper fix. As I and Daniel explained, there are many other
cases where a similar effect of deleting existing socket file can
happen.

I would rather not merge this, but if it's critical, I would add a
FIXME comment around.


-- 
Marc-André Lureau

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

[Pankaj Gupta]

> 
> Hi
> 
> On Thu, Feb 7, 2019 at 11:13 AM Pankaj Gupta <pagupta@redhat.com> wrote:
> >
> >
> > > > > Thanks for your reply. Please find my reply inline.
> > > > >
> > > > > > > > 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.
> > > > > >
> > > > > > Checking the ID is not a useful fix IMHO. Someone could just as
> > > > > > easily
> > > > > > send 2 commands with different IDs and the same socket path.
> > > > > >
> > > > > > A more accurate fix would be to iterate over existing chardevs and
> > > > > > check
> > > > > > whether any of them clash, but even that is useless if you have two
> > > > > > separate QEMU instances and both try to use the same UNIX socket
> > > > > > path.
> > > > > > To deal with that you need to start taking out fcntl locks to
> > > > > > ensure
> > > > > > real mutual exclusion.
> > > > >
> > > > > The reason we are already throwing error "attempt to add duplicate
> > > > > property"
> > > > > implies we are considering "id" as primary key? Even if we throw the
> > > > > error
> > > > > existing chardev should work as before. But this is not the case
> > > > > right
> > > > > now,
> > > > > it just deletes the existing chardev after error.
> > > >
> > > > It deletes the socket "file" (since it overwrites it on chardev
> > > > creation). The existing chardev is not deleted:
> > >
> > > I think this is yet another example of why it is a bad idea for the
> > > qemu_chardev_new API to also open the backend. We should have a
> > > qemu_chardev_new that only does the arg parsing, object creation
> > > and registration. Then have a separate API for actually opening
> > > it.
> >
> > Agree. This looks bigger fix. Till we fix that. Can we accept this patch
> > to avoid deleting/corrupting existing socket "file" if user tries to add
> > chardev with existing same "id"?
> 
> It's not a proper fix. As I and Daniel explained, there are many other
> cases where a similar effect of deleting existing socket file can
> happen.
> 
> I would rather not merge this, but if it's critical, I would add a
> FIXME comment around.
> 

Sure. Sounds good.

Thanks,
Pankaj