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

["Daniel P. Berrangé" <berrange@redhat.com>]

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