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

[Pankaj Gupta <pagupta@redhat.com>]

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