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

[Marc-André Lureau]

Hi

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

qemu-system-x86_64 -qmp stdio
{"QMP": {"version": {"qemu": {"micro": 0, "minor": 0, "major": 3},
"package": "qemu-3.0.0-3.fc29"}, "capabilities": []}}
{"execute":"qmp_capabilities"}
{"return": {}}
 
{"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')"}}
{"execute":"query-chardev"}
{"return": [{"frontend-open": true, "filename": "vc", "label":
"serial0"}, {"frontend-open": true, "filename": "stdio", "label":
"compat_monitor0"}, {"frontend-open": false, "filename":
"disconnected:unix:/tmp/helloworld1,server", "label": "charchannel1"},
{"frontend-open": true, "filename": "vc", "label": "parallel0"}]}


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



-- 
Marc-André Lureau