| Message ID | 1439939415-18180-2-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/18/2015 05:10 PM, 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 <mreitz@redhat.com> > --- > block.c | 25 ++++++++++++++++++------- > include/block/block.h | 2 +- > monitor.c | 6 +++++- > 3 files changed, 24 insertions(+), 9 deletions(-) Would it be better to just have bdrv_get_encrypted_filename() return a g_malloc'd buffer, instead of making the caller do it? Then the buffer can be g_strdup'd to the correct size, instead of over-allocating PATH_MAX bytes when a smaller size will usually do. > > diff --git a/block.c b/block.c > index d088ee0..41b0f85 100644 > --- a/block.c > +++ b/block.c > @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp) > } > } else { > if (bdrv_key_required(bs)) { > + char *enc_filename = g_malloc(PATH_MAX); > + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); > 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); Should you assert(enc_filename) to prove we know we aren't calling error_set("%s", NULL)? After all, bdrv_get_encrypted_filename() can return NULL, but only if no encrypted name is available; while bdrv_key_required() implies that an encrypted name is available. > -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; Again, using g_strdup() here instead of making the caller pass in a buffer might be nicer semantics (certainly fewer places that have to pre-allocate g_malloc0(PATH_MAX) bytes). > +++ b/monitor.c > @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs, > BlockCompletionFunc *completion_cb, > void *opaque) > { > + char *enc_filename; > int err; > > + enc_filename = g_malloc(PATH_MAX); > + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); > monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs), > - bdrv_get_encrypted_filename(bs)); > + enc_filename); > + g_free(enc_filename); And once again, an assert(enc_filename) might be nice to be sure we aren't dealing with a NULL return.
On 31.08.2015 22:50, Eric Blake wrote: > On 08/18/2015 05:10 PM, 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 <mreitz@redhat.com> >> --- >> block.c | 25 ++++++++++++++++++------- >> include/block/block.h | 2 +- >> monitor.c | 6 +++++- >> 3 files changed, 24 insertions(+), 9 deletions(-) > > Would it be better to just have bdrv_get_encrypted_filename() return a > g_malloc'd buffer, instead of making the caller do it? Then the buffer > can be g_strdup'd to the correct size, instead of over-allocating > PATH_MAX bytes when a smaller size will usually do. Probably, yes. I wanted the interface to be similar to the new bdrv_filename(), but since all the callers of bdrv_get_encrypted_filename() would have to allocate the buffer anyway, I probably should pass through bdrv_filename_alloc() (and more on that in my reply to your review of patch 3). >> >> diff --git a/block.c b/block.c >> index d088ee0..41b0f85 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp) >> } >> } else { >> if (bdrv_key_required(bs)) { >> + char *enc_filename = g_malloc(PATH_MAX); >> + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); >> 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); > > Should you assert(enc_filename) to prove we know we aren't calling > error_set("%s", NULL)? After all, bdrv_get_encrypted_filename() can > return NULL, but only if no encrypted name is available; while > bdrv_key_required() implies that an encrypted name is available. Well, in this version, enc_filename will never be NULL because it has been returned by g_malloc(). But it may become necessary with the interface change you suggested (bdrv_get_encrypted_filename() allocating the buffer and returning it), so I'll keep it in mind. Thanks for your review and your suggestions! :-) Max >> -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; > > Again, using g_strdup() here instead of making the caller pass in a > buffer might be nicer semantics (certainly fewer places that have to > pre-allocate g_malloc0(PATH_MAX) bytes). > > >> +++ b/monitor.c >> @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs, >> BlockCompletionFunc *completion_cb, >> void *opaque) >> { >> + char *enc_filename; >> int err; >> >> + enc_filename = g_malloc(PATH_MAX); >> + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); >> monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs), >> - bdrv_get_encrypted_filename(bs)); >> + enc_filename); >> + g_free(enc_filename); > > And once again, an assert(enc_filename) might be nice to be sure we > aren't dealing with a NULL return. >
diff --git a/block.c b/block.c index d088ee0..41b0f85 100644 --- a/block.c +++ b/block.c @@ -2760,10 +2760,13 @@ void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp) } } else { if (bdrv_key_required(bs)) { + char *enc_filename = g_malloc(PATH_MAX); + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); 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); + g_free(enc_filename); } } } @@ -2980,14 +2983,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..a7d1650 100644 --- a/monitor.c +++ b/monitor.c @@ -5292,10 +5292,14 @@ int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs, BlockCompletionFunc *completion_cb, void *opaque) { + char *enc_filename; int err; + enc_filename = g_malloc(PATH_MAX); + bdrv_get_encrypted_filename(bs, enc_filename, PATH_MAX); monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs), - bdrv_get_encrypted_filename(bs)); + enc_filename); + g_free(enc_filename); mon->password_completion_cb = completion_cb; mon->password_opaque = opaque;
Instead of returning a pointer to the filename, copy it into a buffer specified by the caller. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block.c | 25 ++++++++++++++++++------- include/block/block.h | 2 +- monitor.c | 6 +++++- 3 files changed, 24 insertions(+), 9 deletions(-)