From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjM8w-0002Pq-9B for qemu-devel@nongnu.org; Mon, 12 Sep 2016 03:59:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bjM8s-0005mI-Uq for qemu-devel@nongnu.org; Mon, 12 Sep 2016 03:59:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42960) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bjM8s-0005mD-Mv for qemu-devel@nongnu.org; Mon, 12 Sep 2016 03:59:02 -0400 From: Markus Armbruster References: <1473157086-12062-1-git-send-email-dgilbert@redhat.com> <20160906130843.GI10095@redhat.com> <20160906133349.GH2041@work-vm> <87wpilm4ck.fsf@dusky.pond.sub.org> <20160909173340.GN25802@redhat.com> Date: Mon, 12 Sep 2016 09:58:58 +0200 In-Reply-To: <20160909173340.GN25802@redhat.com> (Daniel P. Berrange's message of "Fri, 9 Sep 2016 18:33:40 +0100") Message-ID: <87y42xczwd.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2] qom: Implement qom-get HMP command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: kwolf@redhat.com, qemu-devel@nongnu.org, "Dr. David Alan Gilbert" , lcapitulino@redhat.com, arei.gonglei@huawei.com, pbonzini@redhat.com, afaerber@suse.de "Daniel P. Berrange" writes: > On Fri, Sep 09, 2016 at 06:21:15PM +0200, Markus Armbruster wrote: >> "Dr. David Alan Gilbert" writes: >> >> > * Daniel P. Berrange (berrange@redhat.com) wrote: >> >> IIUC, you switched because string-output-visitor could not handle >> >> complex types. >> >> >> >> I have previously written a text-output-visitor that could do >> >> this correctly, since we have this exact same requirement for >> >> 'qemu-info info' to print out the extra-block specific data >> >> in human friendly format for the LUKS driver. >> >> >> >> https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg01727.html >> > >> > How close to going in is it? It looks from the comments that Eric >> > is thinking about fixing string-output-visitor. >> > I'm not clear why you'd want a text-output-visitor and a string-output-vistior. >> >> Me neither. >> >> In theory, we need two output visitors producing text, one for >> machine-readable output (JSON), and the one for human-readable output. >> >> For the latter, we use the string output visitor. >> >> For the former, we first use the (badly named) QMP output visitor to >> produce a QObject, which is then converted to JSON text by >> qobject_to_json() or qobject_to_json_pretty(). >> >> Each of the two output visitors comes with a matching input visitor that >> reads the same format. >> >> To read JSON text, we first parse it into a QObject with >> json_message_parser_init() & friends, which we then pass to the QMP >> input visitor. >> >> If I read Dan's cover letter correctly, the proposed text output visitor >> competes with the string output visitor. Why not enhance or replace it? > > I guess my original posting was not clear enough about the rational for > the separate visitor. Looking closely at the output format for the > existing string output visitor and comparing to what's proposed for my > text output visitor, you'll see they are completely different output > formats. They both happen to create strings, but that's pretty much > where the similarity ends. So while we could squish the functionality > into one visitor class, it wouldn't really let us share any code - the > visitor would just end up taking different codepaths for each style. > Having these two styles mixed will then make it harder to understand > the logic behind either of them. Begs the question why we need different output formats, but... > There is a specific place where code sharing is useful though - the > formatting of a uint64 in sized notatation ie "1.24 GB", and for > that I'm creating a new shared helper function in util/cutils since > we've already got 2 copies of that logic ! > > Anyway, lets continue this debate on the patch series which actualy > proposes the text-output-visitor, which I'm re-posting in a few > minutes with greater explanation of the new format which should > hopefully clarify why it is justified. ... I agree it should be discussed there, not here.