From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1Nz5ZV-0003mR-Bs
	for qemu-devel@nongnu.org; Tue, 06 Apr 2010 05:59:49 -0400
Received: from [140.186.70.92] (port=51160 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1Nz5ZS-0003li-Su
	for qemu-devel@nongnu.org; Tue, 06 Apr 2010 05:59:48 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1Nz5ZO-0002kp-Vj
	for qemu-devel@nongnu.org; Tue, 06 Apr 2010 05:59:46 -0400
Received: from mx1.redhat.com ([209.132.183.28]:47111)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1Nz5ZO-0002kd-Ny
	for qemu-devel@nongnu.org; Tue, 06 Apr 2010 05:59:42 -0400
Message-ID: <4BBB05EB.5090902@redhat.com>
Date: Tue, 06 Apr 2010 11:59:07 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] [PATCH] cleanup block driver option handling in vl.c
References: <20100405141243.GA24646@lst.de>
In-Reply-To: <20100405141243.GA24646@lst.de>
Content-Type: text/plain; charset=ISO-8859-1
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: Christoph Hellwig <hch@lst.de>
Cc: qemu-devel@nongnu.org

Am 05.04.2010 16:12, schrieb Christoph Hellwig:
> Assign directly to the bdrv_flags variable instead of using
> magic numbers before translating to the BDRV_O_* options.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: qemu/vl.c
> ===================================================================
> --- qemu.orig/vl.c	2010-04-05 11:05:39.042010326 +0200
> +++ qemu/vl.c	2010-04-05 11:12:18.235256380 +0200
> @@ -851,10 +851,8 @@ DriveInfo *drive_init(QemuOpts *opts, vo
>      QEMUMachine *machine = opaque;
>      int max_devs;
>      int index;
> -    int cache;
> -    int aio = 0;
>      int ro = 0;
> -    int bdrv_flags;
> +    int bdrv_flags = 0;
>      int on_read_error, on_write_error;
>      const char *devaddr;
>      DriveInfo *dinfo;
> @@ -863,7 +861,6 @@ DriveInfo *drive_init(QemuOpts *opts, vo
>      *fatal_error = 1;
>  
>      translation = BIOS_ATA_TRANSLATION_AUTO;
> -    cache = 1;
>  
>      if (machine && machine->use_scsi) {
>          type = IF_SCSI;
> @@ -978,11 +975,11 @@ DriveInfo *drive_init(QemuOpts *opts, vo
>  
>      if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
>          if (!strcmp(buf, "off") || !strcmp(buf, "none"))
> -            cache = 0;
> -        else if (!strcmp(buf, "writethrough"))
> -            cache = 1;
> +            bdrv_flags |= BDRV_O_NOCACHE;
>          else if (!strcmp(buf, "writeback"))
> -            cache = 2;
> +            bdrv_flags |= BDRV_O_CACHE_WB;
> +        else if (!strcmp(buf, "writethrough"))
> +            /* this is the default */;

What about following the coding style and using braces here instead of a
semicolon after the comment?

>          else {
>             fprintf(stderr, "qemu: invalid cache option\n");
>             return NULL;
> @@ -991,10 +988,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
>  
>  #ifdef CONFIG_LINUX_AIO
>      if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
> +        if (!strcmp(buf, "native"))
> +            bdrv_flags |= BDRV_O_NATIVE_AIO;
>          if (!strcmp(buf, "threads"))

This needs to become an else if, breaks aio=native otherwise.

> -            aio = 0;
> -        else if (!strcmp(buf, "native"))
> -            aio = 1;
> +            /* this is the default */;

Same as above, but IMHO braces are even mandatory here as every single
line of the code is touched.

>          else {
>             fprintf(stderr, "qemu: invalid aio option\n");
>             return NULL;
> @@ -1169,20 +1166,10 @@ DriveInfo *drive_init(QemuOpts *opts, vo
>          *fatal_error = 0;
>          return NULL;
>      }
> -    bdrv_flags = 0;
>      if (snapshot) {
> -        bdrv_flags |= BDRV_O_SNAPSHOT;
> -        cache = 2; /* always use write-back with snapshot */
> -    }
> -    if (cache == 0) /* no caching */
> -        bdrv_flags |= BDRV_O_NOCACHE;
> -    else if (cache == 2) /* write-back */
> -        bdrv_flags |= BDRV_O_CACHE_WB;
> -
> -    if (aio == 1) {
> -        bdrv_flags |= BDRV_O_NATIVE_AIO;
> -    } else {
> -        bdrv_flags &= ~BDRV_O_NATIVE_AIO;
> +        /* always use write-back with snapshot */
> +        bdrv_flags |= (BDRV_O_SNAPSHOT|BDRV_O_CACHE_WB);
> +        bdrv_flags &= ~BDRV_O_NOCACHE;

I'd prefer to have a bdrv_flags &= ~BDRV_O_CACHE_MASK before the |= line
instead of clearing some random flag that happens to be the only one.

Kevin