| Message ID | 20190129062801.15799-1-pagupta@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] chardev: Avoid adding duplicate chardev | expand |
On Tue, Jan 29, 2019 at 11:58:01AM +0530, Pankaj Gupta wrote: > Hotplugging existing char chardev with qmp, dereferences(removes) > existing chardev. This patch avoids adding a chardev if a chardev > with same id exists. > > RH BZ 1660831: > > # (host) ls -lt /tmp/helloworld* > srwxr-xr-x. /tmp/helloworld1 > srwxr-xr-x. /tmp/helloworld2 > > Before this patch: > > hotplug existed chardev(channel1) in qmp: > {"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')"}} > > # ls -lt /tmp/helloworld* > srwxr-xr-x. 1 root root 0 Dec 19 16:39 /tmp/helloworld2 > > After this patch: > > {"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket", > "data":{"addr":{"type":"unix", "data": {"path": "/tmp/helloworld1"}}}}}} > {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' already exists"}} > > # ls -lt /tmp/helloworld* > srwxr-xr-x. 1 /tmp/helloworld1 > srwxr-xr-x. 1 /tmp/helloworld2 > > Reported-by: Xiaohui Li <xiaohli@redhat.com> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > --- > > v1->v2 > Correct error message - Eric > > chardev/char.c | 6 ++++++ > 1 file changed, 6 insertions(+) Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Hi On Tue, Jan 29, 2019 at 7:36 AM Pankaj Gupta <pagupta@redhat.com> wrote: > > 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. Daniel, you may want to comment (we had a similar discussion about Spice server unix sockets recently) thanks > > RH BZ 1660831: > > # (host) ls -lt /tmp/helloworld* > srwxr-xr-x. /tmp/helloworld1 > srwxr-xr-x. /tmp/helloworld2 > > Before this patch: > > hotplug existed chardev(channel1) in qmp: > {"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')"}} > > # ls -lt /tmp/helloworld* > srwxr-xr-x. 1 root root 0 Dec 19 16:39 /tmp/helloworld2 > > After this patch: > > {"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket", > "data":{"addr":{"type":"unix", "data": {"path": "/tmp/helloworld1"}}}}}} > {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' already exists"}} > > # ls -lt /tmp/helloworld* > srwxr-xr-x. 1 /tmp/helloworld1 > srwxr-xr-x. 1 /tmp/helloworld2 > > Reported-by: Xiaohui Li <xiaohli@redhat.com> > Signed-off-by: Pankaj Gupta <pagupta@redhat.com> > --- > > v1->v2 > Correct error message - Eric > > chardev/char.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/chardev/char.c b/chardev/char.c > index ccba36bafb..cab0d3df16 100644 > --- a/chardev/char.c > +++ b/chardev/char.c > @@ -985,6 +985,12 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, > ChardevReturn *ret; > Chardev *chr; > > + chr = qemu_chr_find(id); > + if (chr) { > + error_setg(errp, "Chardev '%s' already exists", id); > + return NULL; > + } > + > cc = char_get_class(ChardevBackendKind_str(backend->type), errp); > if (!cc) { > return NULL; > -- > 2.14.3 > >
On Wed, Feb 06, 2019 at 05:08:25PM +0100, Marc-André Lureau wrote: > Hi > > On Tue, Jan 29, 2019 at 7:36 AM Pankaj Gupta <pagupta@redhat.com> wrote: > > > > 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. I think I'd really just call this user error and do nothing Regards, Daniel
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. 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 > |:| >
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: 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 > > |:| > >
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
> > > 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
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.
> > 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
diff --git a/chardev/char.c b/chardev/char.c index ccba36bafb..cab0d3df16 100644 --- a/chardev/char.c +++ b/chardev/char.c @@ -985,6 +985,12 @@ ChardevReturn *qmp_chardev_add(const char *id, ChardevBackend *backend, ChardevReturn *ret; Chardev *chr; + chr = qemu_chr_find(id); + if (chr) { + error_setg(errp, "Chardev '%s' already exists", id); + return NULL; + } + cc = char_get_class(ChardevBackendKind_str(backend->type), errp); if (!cc) { return NULL;
Hotplugging existing char chardev with qmp, dereferences(removes) existing chardev. This patch avoids adding a chardev if a chardev with same id exists. RH BZ 1660831: # (host) ls -lt /tmp/helloworld* srwxr-xr-x. /tmp/helloworld1 srwxr-xr-x. /tmp/helloworld2 Before this patch: hotplug existed chardev(channel1) in qmp: {"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')"}} # ls -lt /tmp/helloworld* srwxr-xr-x. 1 root root 0 Dec 19 16:39 /tmp/helloworld2 After this patch: {"execute":"chardev-add","arguments":{"id":"charchannel1","backend":{"type":"socket", "data":{"addr":{"type":"unix", "data": {"path": "/tmp/helloworld1"}}}}}} {"error": {"class": "GenericError", "desc": "Chardev 'charchannel1' already exists"}} # ls -lt /tmp/helloworld* srwxr-xr-x. 1 /tmp/helloworld1 srwxr-xr-x. 1 /tmp/helloworld2 Reported-by: Xiaohui Li <xiaohli@redhat.com> Signed-off-by: Pankaj Gupta <pagupta@redhat.com> --- v1->v2 Correct error message - Eric chardev/char.c | 6 ++++++ 1 file changed, 6 insertions(+)