From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1MTbAd-0006AM-04
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 08:43:43 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1MTbAY-00060A-7l
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 08:43:42 -0400
Received: from [199.232.76.173] (port=59828 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1MTbAX-0005zz-Vh
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 08:43:37 -0400
Received: from mx2.redhat.com ([66.187.237.31]:33166)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <kwolf@redhat.com>) id 1MTbAX-0001UB-Hw
	for qemu-devel@nongnu.org; Wed, 22 Jul 2009 08:43:37 -0400
Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26])
	by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n6MChap6006452
	for <qemu-devel@nongnu.org>; Wed, 22 Jul 2009 08:43:36 -0400
Message-ID: <4A670931.6090008@redhat.com>
Date: Wed, 22 Jul 2009 14:42:25 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH 4/8] qemu-option: factor out parse_option_bool
References: <1248258307-17071-1-git-send-email-kraxel@redhat.com>
	<1248258307-17071-5-git-send-email-kraxel@redhat.com>
In-Reply-To: <1248258307-17071-5-git-send-email-kraxel@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: qemu-devel@nongnu.org

Gerd Hoffmann schrieb:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  qemu-option.c |   34 ++++++++++++++++++++++------------
>  qemu-option.h |    1 +
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/qemu-option.c b/qemu-option.c
> index 646bbad..45f0c8f 100644
> --- a/qemu-option.c
> +++ b/qemu-option.c
> @@ -101,6 +101,23 @@ QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>      return NULL;
>  }
>  
> +int parse_option_bool(const char *name, const char *value, int *ret)

I don't really like this being global. In the end, we want to have only
one parsing mechanism anyway and I don't think it needs to have multiple
source files. So is there any reason not to put your QemuOpts code into
qemu-option.c and leave parsing internals static?

> +{
> +    if (value != NULL) {
> +        if (!strcmp(value, "on")) {
> +            *ret = 1;
> +        } else if (!strcmp(value, "off")) {
> +            *ret = 0;
> +        } else {
> +            fprintf(stderr, "Option '%s': Use 'on' or 'off'\n", name);
> +            return -1;
> +        }
> +    } else {
> +        *ret = 1;
> +    }
> +    return 0;
> +}
> +
>  /*
>   * Sets the value of a parameter in a given option list. The parsing of the
>   * value depends on the type of option:
> @@ -121,6 +138,8 @@ QEMUOptionParameter *get_option_parameter(QEMUOptionParameter *list,
>  int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      const char *value)
>  {
> +    int flag;
> +
>      // Find a matching parameter
>      list = get_option_parameter(list, name);
>      if (list == NULL) {
> @@ -131,18 +150,9 @@ int set_option_parameter(QEMUOptionParameter *list, const char *name,
>      // Process parameter
>      switch (list->type) {
>      case OPT_FLAG:
> -        if (value != NULL) {
> -            if (!strcmp(value, "on")) {
> -                list->value.n = 1;
> -            } else if (!strcmp(value, "off")) {
> -                list->value.n = 0;
> -            } else {
> -                fprintf(stderr, "Option '%s': Use 'on' or 'off'\n", name);
> -                return -1;
> -            }
> -        } else {
> -            list->value.n = 1;
> -        }
> +        if (-1 == parse_option_bool(name, value, &flag))

You're still doing it... ;-)

Kevin