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

[Marc-André Lureau]

Hi

On Thu, Feb 7, 2019 at 11:13 AM Pankaj Gupta <address@hidden> 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.


-- 
Marc-André Lureau