From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50013) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXO9T-0008EE-Ur for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:42:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bXO9P-0002Lx-NV for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:42:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55364) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bXO9P-0002Ls-Ht for qemu-devel@nongnu.org; Wed, 10 Aug 2016 03:42:07 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 CDF5213CF5 for ; Wed, 10 Aug 2016 07:42:06 +0000 (UTC) From: Markus Armbruster References: <1470736094-19194-1-git-send-email-prasanna.kalever@redhat.com> Date: Wed, 10 Aug 2016 09:42:04 +0200 In-Reply-To: <1470736094-19194-1-git-send-email-prasanna.kalever@redhat.com> (Prasanna Kumar Kalever's message of "Tue, 9 Aug 2016 15:18:14 +0530") Message-ID: <87shud3wab.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: Prasanna Kumar Kalever Cc: qemu-devel@nongnu.org, vbellur@redhat.com, jcody@redhat.com, areis@redhat.com 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.