Re: [Qemu-devel] [PATCH v2 2/5] block: Avoid BlockDriverState.filename

[Max Reitz]

On 06.08.2015 04:27, Wen Congyang wrote:
> On 08/06/2015 04:52 AM, Max Reitz wrote:
> > In places which directly pass a filename to the OS, we should not use
> > the filename field at all but exact_filename instead (although the
> > former currently equals the latter if that is set).
> >
> > In qemu-img's map command, we should be using the filename field; but
> > since this commit prepares to remove that field, using exact_filename is
> > fine, too (this is the only user of BlockDriverState.filename which
> > frequently queries that field).
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block.c           | 4 ++--
> >   block/gluster.c   | 2 +-
> >   block/raw-posix.c | 8 ++++----
> >   block/raw-win32.c | 4 ++--
> >   qemu-img.c        | 2 +-
> >   5 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index e7dd2f1..5a1dc16 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -918,8 +918,8 @@ static int bdrv_open_common(BlockDriverState *bs, 
> > BlockDriverState *file,
> >       if (ret < 0) {
> >           if (local_err) {
> >               error_propagate(errp, local_err);
> > -        } else if (bs->filename[0]) {
> > -            error_setg_errno(errp, -ret, "Could not open '%s'", bs->filename);
> > +        } else if (bs->exact_filename[0]) {
> > +            error_setg_errno(errp, -ret, "Could not open '%s'", 
> > bs->exact_filename);
> >           } else {
> >               error_setg_errno(errp, -ret, "Could not open image");
> >           }
> > diff --git a/block/gluster.c b/block/gluster.c
> > index 1eb3a8c..176682b 100644
> > --- a/block/gluster.c
> > +++ b/block/gluster.c
> > @@ -358,7 +358,7 @@ static int qemu_gluster_reopen_prepare(BDRVReopenState 
> > *state,
> >
> >       gconf = g_new0(GlusterConf, 1);
> >
> > -    reop_s->glfs = qemu_gluster_init(gconf, state->bs->filename, errp);
> > +    reop_s->glfs = qemu_gluster_init(gconf, state->bs->exact_filename, errp);
> >       if (reop_s->glfs == NULL) {
> >           ret = -errno;
> >           goto exit;
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 855febe..b61c11f 100644
> > --- a/block/raw-posix.c
> > +++ b/block/raw-posix.c
> > @@ -671,7 +671,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
> >       /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */
> >       if (raw_s->fd == -1) {
> >           assert(!(raw_s->open_flags & O_CREAT));
> > -        raw_s->fd = qemu_open(state->bs->filename, raw_s->open_flags);
> > +        raw_s->fd = qemu_open(state->bs->exact_filename, raw_s->open_flags);
>
> In raw_open_common(), we call raw_normalize_devicepath(). Why don't we call it
> here?

Ideally because bs->exact_filename is normalized already, whereas 
raw_open_common() takes the filename from the options QDict (which was 
specified by the user). The question now is whether bs->exact_filename 
is indeed normalized.

Looking into bdrv_open_common(), where that field is initially set, it 
doesn't look that way.

One way to fix this would be to implement bdrv_refresh_filename() for 
raw-posix. However, then we still have the issue of bs->filename usage 
in raw_open_common() (for an error message, although in reality, that 
message can only appear on Linux where raw_normalize_devicepath() won't 
do anything).

So the easy way to fix it would be to copy the normalized filename into 
bs->filename at the beginning of raw_open_common(). I'll think about it, 
but in any case, I think this issue is not related to this series and 
the fix will work independently.

I'll work something out, thanks for spotting this!

Max

> Thanks
> Wen Congyang
>
> >           if (raw_s->fd == -1) {
> >               error_setg_errno(errp, errno, "Could not reopen file");
> >               ret = -1;
> > @@ -2195,7 +2195,7 @@ static int fd_open(BlockDriverState *bs)
> >               DPRINTF("No floppy (open delayed)\n");
> >               return -EIO;
> >           }
> > -        s->fd = qemu_open(bs->filename, s->open_flags & ~O_NONBLOCK);
> > +        s->fd = qemu_open(bs->exact_filename, s->open_flags & ~O_NONBLOCK);
> >           if (s->fd < 0) {
> >               s->fd_error_time = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> >               s->fd_got_error = 1;
> > @@ -2486,7 +2486,7 @@ static void floppy_eject(BlockDriverState *bs, bool 
> > eject_flag)
> >           qemu_close(s->fd);
> >           s->fd = -1;
> >       }
> > -    fd = qemu_open(bs->filename, s->open_flags | O_NONBLOCK);
> > +    fd = qemu_open(bs->exact_filename, s->open_flags | O_NONBLOCK);
> >       if (fd >= 0) {
> >           if (ioctl(fd, FDEJECT, 0) < 0)
> >               perror("FDEJECT");
> > @@ -2710,7 +2710,7 @@ static int cdrom_reopen(BlockDriverState *bs)
> >        */
> >       if (s->fd >= 0)
> >           qemu_close(s->fd);
> > -    fd = qemu_open(bs->filename, s->open_flags, 0644);
> > +    fd = qemu_open(bs->exact_filename, s->open_flags, 0644);
> >       if (fd < 0) {
> >           s->fd = -1;
> >           return -EIO;
> > diff --git a/block/raw-win32.c b/block/raw-win32.c
> > index 68f2338..5c8a894 100644
> > --- a/block/raw-win32.c
> > +++ b/block/raw-win32.c
> > @@ -417,7 +417,7 @@ static void raw_close(BlockDriverState *bs)
> >
> >       CloseHandle(s->hfile);
> >       if (bs->open_flags & BDRV_O_TEMPORARY) {
> > -        unlink(bs->filename);
> > +        unlink(bs->exact_filename);
> >       }
> >   }
> >
> > @@ -485,7 +485,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState 
> > *bs)
> >                                                 DWORD * high);
> >       get_compressed_t get_compressed;
> >       struct _stati64 st;
> > -    const char *filename = bs->filename;
> > +    const char *filename = bs->exact_filename;
> >       /* WinNT support GetCompressedFileSize to determine allocate size */
> >       get_compressed =
> >           (get_compressed_t) GetProcAddress(GetModuleHandle("kernel32"),
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 75f4ee4..3d5587a 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -2155,7 +2155,7 @@ static void dump_map_entry(OutputFormat output_format, 
> > MapEntry *e,
> >           }
> >           if ((e->flags & (BDRV_BLOCK_DATA|BDRV_BLOCK_ZERO)) == 
> > BDRV_BLOCK_DATA) {
> >               printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
> > -                   e->start, e->length, e->offset, e->bs->filename);
> > +                   e->start, e->length, e->offset, e->bs->exact_filename);
> >           }
> >           /* This format ignores the distinction between 0, ZERO and ZERO|DATA.
> >            * Modify the flags here to allow more coalescing.