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 <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.

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 :|