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

[Pankaj Gupta <pagupta@redhat.com>]

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