From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47504) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UdG9F-0001Ct-0y for qemu-devel@nongnu.org; Fri, 17 May 2013 04:36:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UdG9A-0005lB-3s for qemu-devel@nongnu.org; Fri, 17 May 2013 04:36:20 -0400 Received: from e23smtp05.au.ibm.com ([202.81.31.147]:55350) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UdG99-0005kd-HG for qemu-devel@nongnu.org; Fri, 17 May 2013 04:36:16 -0400 Received: from /spool/local by e23smtp05.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 17 May 2013 18:30:42 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp03.au.ibm.com (Postfix) with ESMTP id D2352357804A for ; Fri, 17 May 2013 18:36:03 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4H8LpBO23986218 for ; Fri, 17 May 2013 18:21:52 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4H8a2QR008238 for ; Fri, 17 May 2013 18:36:02 +1000 Message-ID: <5195EBDF.5050501@linux.vnet.ibm.com> Date: Fri, 17 May 2013 16:35:43 +0800 From: Lei Li MIME-Version: 1.0 References: <1368589953-22455-1-git-send-email-lilei@linux.vnet.ibm.com> <51931F75.8040107@redhat.com> <5193388F.1020609@linux.vnet.ibm.com> In-Reply-To: <5193388F.1020609@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] chardev: Get filename for new qapi backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gerd Hoffmann Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org On 05/15/2013 03:26 PM, Lei Li wrote: > On 05/15/2013 01:39 PM, Gerd Hoffmann wrote: >> On 05/15/13 05:52, Lei Li wrote: >>> This patch add the filename when the new qapi backend init from opts. >>> >>> Commit 2c5f488293c7d0cd095635c74157c2526e2c4947 add support for >>> qapi-based chardev initialization, but miss the filename of the >>> char device as below: >>> >>> (qemu) info chardev >>> parallel0: filename=(null) >>> serial0: filename=(null) >>> compat_monitor0: filename=(null) >>> @@ -3276,6 +3276,7 @@ CharDriverState >>> *qemu_chr_new_from_opts(QemuOpts *opts, >>> ChardevReturn *ret = NULL; >>> const char *id = qemu_opts_id(opts); >>> const char *bid = NULL; >>> + char *filename = g_strdup(qemu_opt_get(opts, "backend")); >>> if (qemu_opt_get_bool(opts, "mux", 0)) { >>> bid = g_strdup_printf("%s-base", id); >>> @@ -3308,6 +3309,7 @@ CharDriverState >>> *qemu_chr_new_from_opts(QemuOpts *opts, >>> } >>> chr = qemu_chr_find(id); >>> + chr->filename = filename; >> That should happen in qmp_chardev_add(), so filename is set consistently >> for every chardev no matter how it gets created. > > Yeah, I was hesitant to add it here for that it does not looks very > logical... > But I am not sure about the best place for it.. > >> Take care to not overwrite filename, some functions >> (qmp_chardev_open_socket for example) do set chr->filename already. > > Yes, for the char device pipe, pty, socket, they set chr->filename in > their > own open functions. But I think the overwrite will not happen because > it will not get "backend" by qemu_opt_get() for these chardevs. > >> >> Maybe it's better to put that into the individual qmp_chardev_open_* >> functions anyway. > > OK, I'll have a try and see if everyone like it, I don't have better > idea. > Hi Gerd, As I tried to add the filename into individual open functions, I think it might not be a good idea. For example, chardev backend file and console have the same open function qemu_chr_open_win_file(). Then we may need to add flag to distinguish them. And we already parse every backend into QemuOpts, if we want to put the filename setting job into open functions (by hardcoding) seems a little overdone? So why not just set it in qemu_chr_new_from_opts(), by qemu_opt_get() directly? >> >> cheers, >> Gerd >> >> >> > > -- Lei