Re: [Qemu-devel] [PATCH 2/4] raw-posix: add a raw_open_common helper

[Glauber Costa <glommer@redhat.com>]

On Mon, May 25, 2009 at 09:59:22AM +0200, Christoph Hellwig wrote:
> 
> raw_open and hdev_open contain the same basic logic.  Add a new
> raw_open_common helper containing the guts of the open routine
> and call it from raw_open and hdev_open.
> 
> We use the new open_flags field in BDRVRawState to allow passing
> additional open flags to raw_open_common from both.
> 
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: qemu/block/raw-posix.c
> ===================================================================
> --- qemu.orig/block/raw-posix.c	2009-05-25 09:20:13.949939473 +0200
> +++ qemu/block/raw-posix.c	2009-05-25 09:21:56.307963685 +0200
> @@ -124,7 +124,8 @@ static int cd_open(BlockDriverState *bs)
>  
>  static int raw_is_inserted(BlockDriverState *bs);
>  
> -static int raw_open(BlockDriverState *bs, const char *filename, int flags)
> +static int raw_open_common(BlockDriverState *bs, const char *filename,
> +        int flags)
>  {
>      BDRVRawState *s = bs->opaque;
>      int fd, ret;
> @@ -140,8 +141,6 @@ static int raw_open(BlockDriverState *bs
>          s->open_flags |= O_RDONLY;
>          bs->read_only = 1;
>      }
> -    if (flags & BDRV_O_CREAT)
> -        s->open_flags |= O_CREAT | O_TRUNC;
>  
>      /* Use O_DSYNC for write-through caching, no flags for write-back caching,
>       * and O_DIRECT for no caching. */
> @@ -150,8 +149,7 @@ static int raw_open(BlockDriverState *bs
>      else if (!(flags & BDRV_O_CACHE_WB))
>          s->open_flags |= O_DSYNC;
>  
> -    s->type = FTYPE_FILE;
> -
> +    s->fd = -1;
>      fd = open(filename, s->open_flags, 0644);
>      if (fd < 0) {
>          ret = -errno;
it is not equivalent to the current code, while you claim it is.
this one will return EROFS to the external world, while old code
would return EACCES. If there is any user relying on that return value,
we're screwed. And as a matter of fact, it appears to be (bdrv_open2)

>  #ifdef CONFIG_COCOA
>      if (strstart(filename, "/dev/cdrom", NULL)) {
> @@ -972,19 +979,6 @@ static int hdev_open(BlockDriverState *b
>              IOObjectRelease( mediaIterator );
>      }
>  #endif
> -    s->open_flags |= O_BINARY;
> -    if ((flags & BDRV_O_ACCESS) == O_RDWR) {
> -        s->open_flags |= O_RDWR;
> -    } else {
> -        s->open_flags |= O_RDONLY;
> -        bs->read_only = 1;
> -    }
> -    /* Use O_DSYNC for write-through caching, no flags for write-back caching,
> -     * and O_DIRECT for no caching. */
> -    if ((flags & BDRV_O_NOCACHE))
> -        s->open_flags |= O_DIRECT;
> -    else if (!(flags & BDRV_O_CACHE_WB))
> -        s->open_flags |= O_DSYNC;
>  
What happened to those flags? Are you just throwing them away?