From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:53156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScE8z-0000Kr-3E for qemu-devel@nongnu.org; Wed, 06 Jun 2012 07:11:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ScE8t-0006VJ-U9 for qemu-devel@nongnu.org; Wed, 06 Jun 2012 07:11:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58990) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScE8t-0006V8-MB for qemu-devel@nongnu.org; Wed, 06 Jun 2012 07:11:11 -0400 Message-ID: <4FCF3B10.10102@redhat.com> Date: Wed, 06 Jun 2012 13:12:16 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1337683555-13301-1-git-send-email-lersek@redhat.com> <1337683555-13301-5-git-send-email-lersek@redhat.com> <4FCE7636.6090500@redhat.com> In-Reply-To: <4FCE7636.6090500@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 04/16] qapi: introduce OptsVisitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org, Michael Roth Thank you very much for the review! One question below (and maybe some more later in response to other parts of the review): On 06/05/12 23:12, Paolo Bonzini wrote: > Il 22/05/2012 12:45, Laszlo Ersek ha scritto: >> Optarg values can be of scalar types str / bool / int / size. > > Michael Roth recently added Visitor interfaces for uint*_t and int*_t, > they look like they would simplify the patches very nicely. They do the > range checking automatically and only need a type_int callback in the > visitor. > > You can get the patch from http://patchwork.ozlabs.org/patch/150427/, > feel free to include it at the beginning of your series. The unsigned functions don't check if the int64_t value to be assigned is negative. What happens in such a case is fully defined, but did Michael really intend the wraparound to unsigned? I thought it was a silent requirement for the int parser to return a non-negative int (which is something I also implemented in opts_type_int() [*]), but the signed visit_type_XXX() functions do check for the negative limit. I'm confused. [*] see the comment in opts-visitor.h: /* Contrarily to qemu-option.c::parse_option_number(), OptsVisitor's "int" * parser relies on strtoll() instead of strtoull(). Consequences: * - string representations of negative numbers are rejected (parsed values * continue to be non-negative), * - values above INT64_MAX or LLONG_MAX are rejected. */ That's also why I dropped/omitted the "< 0" checks in net_init_nic() [v1 09/16], net_init_dump() [v1 10/16] and net_init_vde() [v1 13/16]. If you want I can go through all the integer fields I introduced in [v1 06/16] "qapi schema: add Netdev types" and classify each as strictly as possible (and then remove the checks being obviated). Thanks! Laszlo