From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49743) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXTxw-00011J-EA for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:54:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXTxr-0007M5-14 for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:54:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35340) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXTxq-0007Le-OF for qemu-devel@nongnu.org; Wed, 10 Aug 2016 09:54:34 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BDDFC81F07 for ; Wed, 10 Aug 2016 13:54:33 +0000 (UTC) From: Markus Armbruster References: <1470736094-19194-1-git-send-email-prasanna.kalever@redhat.com> <87shud3wab.fsf@dusky.pond.sub.org> <20160810130409.GG5270@localhost.localdomain> Date: Wed, 10 Aug 2016 15:54:31 +0200 In-Reply-To: <20160810130409.GG5270@localhost.localdomain> (Jeff Cody's message of "Wed, 10 Aug 2016 09:04:09 -0400") Message-ID: <87twes91bc.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v1 1/1] block/gluster: fix port type in the QAPI options list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jeff Cody Cc: areis@redhat.com, Prasanna Kumar Kalever , qemu-devel@nongnu.org, vbellur@redhat.com Jeff Cody writes: > On Wed, Aug 10, 2016 at 09:42:04AM +0200, Markus Armbruster wrote: >> Prasanna Kumar Kalever writes: >> >> > After introduction of qapi schema in gluster block driver code, the port >> > type is now string as per InetSocketAddress >> > >> > { 'struct': 'InetSocketAddress', >> > 'data': { >> > 'host': 'str', >> > 'port': 'str', >> > '*to': 'uint16', >> > '*ipv4': 'bool', >> > '*ipv6': 'bool' } } >> > >> > but the current code still treats it as QEMU_OPT_NUMBER, hence fixing port >> > to accept QEMU_OPT_STRING. >> > >> > Credits: Markus Armbruster >> >> Commonly written as >> Suggested-by: Markus Armbruster >> >> > Signed-off-by: Prasanna Kumar Kalever >> > --- >> > block/gluster.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/block/gluster.c b/block/gluster.c >> > index edde1ad..e6afa48 100644 >> > --- a/block/gluster.c >> > +++ b/block/gluster.c >> > @@ -161,7 +161,7 @@ static QemuOptsList runtime_tcp_opts = { >> > }, >> > { >> > .name = GLUSTER_OPT_PORT, >> > - .type = QEMU_OPT_NUMBER, >> > + .type = QEMU_OPT_STRING, >> > .help = "port number on which glusterd is listening (default 24007)", >> > }, >> > { >> >> The difference between QEMU_OPT_NUMBER and QEMU_OPT_STRING: >> >> * The string value is stored for both. For QEMU_OPT_NUMBER, we >> additionally parse the string as decimal number (this can fail, >> obviously), and store the result as uint64_t. See qemu_opt_parse(). >> >> * qemu_opt_get() & friends return the stored string for both. >> >> * qemu_opt_get_number() & friends require QEMU_OPT_NUMBER and return the >> stored number. >> >> * qemu_opts_print() prints the stored string (with comma doubled) for >> QEMU_OPT_STRING, and the stored number for QEMU_OPT_NUMBER. >> >> Your patch works, because: >> >> * We get the value only with qemu_opt_get(). The only effect we get >> from QEMU_OPT_NUMBER is qemu_opt_parse() failure. >> >> * "[PATCH v2 1/1] block/gluster: improve defense over string to int >> conversion" fixes the conversion port string to port number to detect >> errors. With QEMU_OPT_NUMBER, this can't actually fail, because >> qemu_opt_parse() fails first. With QEMU_OPT_STRING, it can. >> >> The commit message should explain this. >> >> I'd squash the two patches together, because a decent commit message for >> the squash will probably be simpler than separate ones. > > Are these two patches intended for 2.7? Back when I suggested this work, I had hoped it could still make 2.7 as a followup fix, but 1. the bug has turned out to be merely latent , and 2. it's -rc3. I guess we need to punt them to 2.8.