From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:36627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1grgrb-0002iQ-0l for qemu-devel@nongnu.org; Thu, 07 Feb 2019 05:25:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1grgrZ-0001iw-Mt for qemu-devel@nongnu.org; Thu, 07 Feb 2019 05:24:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54874) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1grgrZ-0001T4-CY for qemu-devel@nongnu.org; Thu, 07 Feb 2019 05:24:57 -0500 Date: Thu, 7 Feb 2019 05:24:19 -0500 (EST) From: Pankaj Gupta Message-ID: <206743624.70377131.1549535059368.JavaMail.zimbra@redhat.com> In-Reply-To: References: <20190129062801.15799-1-pagupta@redhat.com> <20190206162901.GW12331@redhat.com> <1385614518.70345867.1549524112156.JavaMail.zimbra@redhat.com> <20190207093400.GB19438@redhat.com> <1487450667.70375757.1549534386311.JavaMail.zimbra@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] chardev: Avoid adding duplicate chardev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Cc: Daniel =?utf-8?Q?P=2E_Berrang=C3=A9?= , Paolo Bonzini , QEMU , xiaohli@redhat.com > > Hi > > On Thu, Feb 7, 2019 at 11:13 AM Pankaj Gupta 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