Re: [Qemu-devel] [PATCH v2 1/5] block: Change bdrv_get_encrypted_filename()

[Max Reitz]

On 06.08.2015 04:01, Wen Congyang wrote:
> On 08/06/2015 04:52 AM, Max Reitz wrote:
> > Instead of returning a pointer to the filename, copy it into a buffer
> > specified by the caller.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block.c               | 24 +++++++++++++++++-------
> >   include/block/block.h |  2 +-
> >   monitor.c             |  4 +++-
> >   3 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index d088ee0..e7dd2f1 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2760,10 +2760,12 @@ void bdrv_add_key(BlockDriverState *bs, const char 
> > *key, Error **errp)
> >           }
> >       } else {
> >           if (bdrv_key_required(bs)) {
> > +            char enc_filename[PATH_MAX];
>
> I think it's better to use g_malloc() here.
>
> > +            bdrv_get_encrypted_filename(bs, enc_filename, 
> > sizeof(enc_filename));
> >               error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
> >                         "'%s' (%s) is encrypted",
> >                         bdrv_get_device_or_node_name(bs),
> > -                      bdrv_get_encrypted_filename(bs));
> > +                      enc_filename);
> >           }
> >       }
> >   }
> > @@ -2980,14 +2982,22 @@ bool bdrv_can_write_zeroes_with_unmap(BlockDriverState 
> > *bs)
> >       return false;
> >   }
> >
> > -const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
> > +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz)
> >   {
> > -    if (bs->backing_hd && bs->backing_hd->encrypted)
> > -        return bs->backing_file;
> > -    else if (bs->encrypted)
> > -        return bs->filename;
> > -    else
> > +    if (sz > INT_MAX) {
> > +        sz = INT_MAX;
> > +    }
> > +
> > +    if (bs->backing_hd && bs->backing_hd->encrypted) {
> > +        pstrcpy(dest, sz, bs->backing_file);
> > +        return dest;
> > +    } else if (bs->encrypted) {
> > +        pstrcpy(dest, sz, bs->filename);
> > +        return dest;
> > +    } else {
> > +        dest[0] = '\0';
> >           return NULL;
> > +    }
> >   }
> >
> >   void bdrv_get_backing_filename(BlockDriverState *bs,
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 37916f7..a78e4f1 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -430,7 +430,7 @@ void bdrv_round_to_clusters(BlockDriverState *bs,
> >                               int64_t *cluster_sector_num,
> >                               int *cluster_nb_sectors);
> >
> > -const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
> > +char *bdrv_get_encrypted_filename(BlockDriverState *bs, char *dest, size_t sz);
> >   void bdrv_get_backing_filename(BlockDriverState *bs,
> >                                  char *filename, int filename_size);
> >   void bdrv_get_full_backing_filename(BlockDriverState *bs,
> > diff --git a/monitor.c b/monitor.c
> > index aeea2b5..cfdf781 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -5292,10 +5292,12 @@ int monitor_read_bdrv_key_start(Monitor *mon, 
> > BlockDriverState *bs,
> >                                   BlockCompletionFunc *completion_cb,
> >                                   void *opaque)
> >   {
> > +    char enc_filename[PATH_MAX];
>
> same too.

You're right. I'll change it (in every patch of this series that pattern 
appears in).

Thanks for reviewing!

Max

> Thanks
> Wen Congyang
>
> >       int err;
> >
> > +    bdrv_get_encrypted_filename(bs, enc_filename, sizeof(enc_filename));
> >       monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
> > -                   bdrv_get_encrypted_filename(bs));
> > +                   enc_filename);
> >
> >       mon->password_completion_cb = completion_cb;
> >       mon->password_opaque = opaque;