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