Add const modifier to passphrases,
and remove redundant stack variable passphrase_len.
Signed-off-by: Or Ozeri <oro@il.ibm.com>
---
block/rbd.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/block/rbd.c b/block/rbd.c
index f826410f40..e575105e6d 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
#ifdef LIBRBD_SUPPORTS_ENCRYPTION
static int qemu_rbd_convert_luks_options(
RbdEncryptionOptionsLUKSBase *luks_opts,
- char **passphrase,
+ const char **passphrase,
size_t *passphrase_len,
Error **errp)
{
@@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
static int qemu_rbd_convert_luks_create_options(
RbdEncryptionCreateOptionsLUKSBase *luks_opts,
rbd_encryption_algorithm_t *alg,
- char **passphrase,
+ const char **passphrase,
size_t *passphrase_len,
Error **errp)
{
@@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
Error **errp)
{
int r = 0;
- g_autofree char *passphrase = NULL;
- size_t passphrase_len;
+ g_autofree const char *passphrase = NULL;
rbd_encryption_format_t format;
rbd_encryption_options_t opts;
rbd_encryption_luks1_format_options_t luks_opts;
@@ -407,12 +406,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
opts_size = sizeof(luks_opts);
r = qemu_rbd_convert_luks_create_options(
qapi_RbdEncryptionCreateOptionsLUKS_base(&encrypt->u.luks),
- &luks_opts.alg, &passphrase, &passphrase_len, errp);
+ &luks_opts.alg, &passphrase, &luks_opts.passphrase_size,
+ errp);
if (r < 0) {
return r;
}
luks_opts.passphrase = passphrase;
- luks_opts.passphrase_size = passphrase_len;
break;
}
case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
@@ -423,12 +422,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
r = qemu_rbd_convert_luks_create_options(
qapi_RbdEncryptionCreateOptionsLUKS2_base(
&encrypt->u.luks2),
- &luks2_opts.alg, &passphrase, &passphrase_len, errp);
+ &luks2_opts.alg, &passphrase, &luks2_opts.passphrase_size,
+ errp);
if (r < 0) {
return r;
}
luks2_opts.passphrase = passphrase;
- luks2_opts.passphrase_size = passphrase_len;
break;
}
default: {
@@ -466,8 +465,7 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
Error **errp)
{
int r = 0;
- g_autofree char *passphrase = NULL;
- size_t passphrase_len;
+ g_autofree const char *passphrase = NULL;
rbd_encryption_luks1_format_options_t luks_opts;
rbd_encryption_luks2_format_options_t luks2_opts;
rbd_encryption_format_t format;
@@ -482,12 +480,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
opts_size = sizeof(luks_opts);
r = qemu_rbd_convert_luks_options(
qapi_RbdEncryptionOptionsLUKS_base(&encrypt->u.luks),
- &passphrase, &passphrase_len, errp);
+ &passphrase, &luks_opts.passphrase_size, errp);
if (r < 0) {
return r;
}
luks_opts.passphrase = passphrase;
- luks_opts.passphrase_size = passphrase_len;
break;
}
case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
@@ -497,12 +494,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
opts_size = sizeof(luks2_opts);
r = qemu_rbd_convert_luks_options(
qapi_RbdEncryptionOptionsLUKS2_base(&encrypt->u.luks2),
- &passphrase, &passphrase_len, errp);
+ &passphrase, &luks2_opts.passphrase_size, errp);
if (r < 0) {
return r;
}
luks2_opts.passphrase = passphrase;
- luks2_opts.passphrase_size = passphrase_len;
break;
}
default: {
--
2.25.1On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> Add const modifier to passphrases,
> and remove redundant stack variable passphrase_len.
>
> Signed-off-by: Or Ozeri <oro@il.ibm.com>
> ---
> block/rbd.c | 24 ++++++++++--------------
> 1 file changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index f826410f40..e575105e6d 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> static int qemu_rbd_convert_luks_options(
> RbdEncryptionOptionsLUKSBase *luks_opts,
> - char **passphrase,
> + const char **passphrase,
> size_t *passphrase_len,
> Error **errp)
> {
> @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> static int qemu_rbd_convert_luks_create_options(
> RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> rbd_encryption_algorithm_t *alg,
> - char **passphrase,
> + const char **passphrase,
> size_t *passphrase_len,
> Error **errp)
> {
> @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> Error **errp)
> {
> int r = 0;
> - g_autofree char *passphrase = NULL;
> - size_t passphrase_len;
> + g_autofree const char *passphrase = NULL;
This looks wierd. If it is as const string, why are
we free'ing it ? Either want g_autofree, or const,
but not both.
> rbd_encryption_format_t format;
> rbd_encryption_options_t opts;
> rbd_encryption_luks1_format_options_t luks_opts;
> @@ -407,12 +406,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> opts_size = sizeof(luks_opts);
> r = qemu_rbd_convert_luks_create_options(
> qapi_RbdEncryptionCreateOptionsLUKS_base(&encrypt->u.luks),
> - &luks_opts.alg, &passphrase, &passphrase_len, errp);
> + &luks_opts.alg, &passphrase, &luks_opts.passphrase_size,
> + errp);
> if (r < 0) {
> return r;
> }
> luks_opts.passphrase = passphrase;
> - luks_opts.passphrase_size = passphrase_len;
> break;
> }
> case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> @@ -423,12 +422,12 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> r = qemu_rbd_convert_luks_create_options(
> qapi_RbdEncryptionCreateOptionsLUKS2_base(
> &encrypt->u.luks2),
> - &luks2_opts.alg, &passphrase, &passphrase_len, errp);
> + &luks2_opts.alg, &passphrase, &luks2_opts.passphrase_size,
> + errp);
> if (r < 0) {
> return r;
> }
> luks2_opts.passphrase = passphrase;
> - luks2_opts.passphrase_size = passphrase_len;
> break;
> }
> default: {
> @@ -466,8 +465,7 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
> Error **errp)
> {
> int r = 0;
> - g_autofree char *passphrase = NULL;
> - size_t passphrase_len;
> + g_autofree const char *passphrase = NULL;
> rbd_encryption_luks1_format_options_t luks_opts;
> rbd_encryption_luks2_format_options_t luks2_opts;
> rbd_encryption_format_t format;
> @@ -482,12 +480,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
> opts_size = sizeof(luks_opts);
> r = qemu_rbd_convert_luks_options(
> qapi_RbdEncryptionOptionsLUKS_base(&encrypt->u.luks),
> - &passphrase, &passphrase_len, errp);
> + &passphrase, &luks_opts.passphrase_size, errp);
> if (r < 0) {
> return r;
> }
> luks_opts.passphrase = passphrase;
> - luks_opts.passphrase_size = passphrase_len;
> break;
> }
> case RBD_IMAGE_ENCRYPTION_FORMAT_LUKS2: {
> @@ -497,12 +494,11 @@ static int qemu_rbd_encryption_load(rbd_image_t image,
> opts_size = sizeof(luks2_opts);
> r = qemu_rbd_convert_luks_options(
> qapi_RbdEncryptionOptionsLUKS2_base(&encrypt->u.luks2),
> - &passphrase, &passphrase_len, errp);
> + &passphrase, &luks2_opts.passphrase_size, errp);
> if (r < 0) {
> return r;
> }
> luks2_opts.passphrase = passphrase;
> - luks2_opts.passphrase_size = passphrase_len;
> break;
> }
> default: {
> --
> 2.25.1
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > Add const modifier to passphrases,
> > and remove redundant stack variable passphrase_len.
> >
> > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > ---
> > block/rbd.c | 24 ++++++++++--------------
> > 1 file changed, 10 insertions(+), 14 deletions(-)
> >
> > diff --git a/block/rbd.c b/block/rbd.c
> > index f826410f40..e575105e6d 100644
> > --- a/block/rbd.c
> > +++ b/block/rbd.c
> > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > static int qemu_rbd_convert_luks_options(
> > RbdEncryptionOptionsLUKSBase *luks_opts,
> > - char **passphrase,
> > + const char **passphrase,
> > size_t *passphrase_len,
> > Error **errp)
> > {
> > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > static int qemu_rbd_convert_luks_create_options(
> > RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > rbd_encryption_algorithm_t *alg,
> > - char **passphrase,
> > + const char **passphrase,
> > size_t *passphrase_len,
> > Error **errp)
> > {
> > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > Error **errp)
> > {
> > int r = 0;
> > - g_autofree char *passphrase = NULL;
> > - size_t passphrase_len;
> > + g_autofree const char *passphrase = NULL;
>
> This looks wierd. If it is as const string, why are
> we free'ing it ? Either want g_autofree, or const,
> but not both.
Just curious, is it a requirement imposed by g_autofree? Otherwise
pointer constness and pointee lifetime are completely orthogonal and
freeing (or, in this case, wanting to auto-free) an object referred to
by a const pointer seems perfectly fine to me.
Thanks,
Ilya
On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > Add const modifier to passphrases,
> > > and remove redundant stack variable passphrase_len.
> > >
> > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > ---
> > > block/rbd.c | 24 ++++++++++--------------
> > > 1 file changed, 10 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/block/rbd.c b/block/rbd.c
> > > index f826410f40..e575105e6d 100644
> > > --- a/block/rbd.c
> > > +++ b/block/rbd.c
> > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > static int qemu_rbd_convert_luks_options(
> > > RbdEncryptionOptionsLUKSBase *luks_opts,
> > > - char **passphrase,
> > > + const char **passphrase,
> > > size_t *passphrase_len,
> > > Error **errp)
> > > {
> > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > static int qemu_rbd_convert_luks_create_options(
> > > RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > rbd_encryption_algorithm_t *alg,
> > > - char **passphrase,
> > > + const char **passphrase,
> > > size_t *passphrase_len,
> > > Error **errp)
> > > {
> > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > Error **errp)
> > > {
> > > int r = 0;
> > > - g_autofree char *passphrase = NULL;
> > > - size_t passphrase_len;
> > > + g_autofree const char *passphrase = NULL;
> >
> > This looks wierd. If it is as const string, why are
> > we free'ing it ? Either want g_autofree, or const,
> > but not both.
>
> Just curious, is it a requirement imposed by g_autofree? Otherwise
> pointer constness and pointee lifetime are completely orthogonal and
> freeing (or, in this case, wanting to auto-free) an object referred to
> by a const pointer seems perfectly fine to me.
Free'ing a const point is not OK
$ cat c.c
#include <stdlib.h>
void bar(const char *foo) {
free(foo);
}
$ gcc -Wall -c c.c
c.c: In function ‘bar’:
c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
5 | free(foo);
| ^~~
In file included from c.c:2:
/usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
568 | extern void free (void *__ptr) __THROW;
| ~~~~~~^~~~~
The g_autofree happens to end up hiding this warning, because the const
annotation isn't propagated to the registere callback, but that doesn't
mean we should do that.
When a programmer sees a variable annotated const, they expect that
either someone else is responsible for free'ing it, or that the data
is statically initialized or stack allocated and thus doesn't need
free'ing. So g_autofree + const is just wrong.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Thu, Jan 12, 2023 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> > On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >
> > > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > > Add const modifier to passphrases,
> > > > and remove redundant stack variable passphrase_len.
> > > >
> > > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > > ---
> > > > block/rbd.c | 24 ++++++++++--------------
> > > > 1 file changed, 10 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > index f826410f40..e575105e6d 100644
> > > > --- a/block/rbd.c
> > > > +++ b/block/rbd.c
> > > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > > #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > > static int qemu_rbd_convert_luks_options(
> > > > RbdEncryptionOptionsLUKSBase *luks_opts,
> > > > - char **passphrase,
> > > > + const char **passphrase,
> > > > size_t *passphrase_len,
> > > > Error **errp)
> > > > {
> > > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > > static int qemu_rbd_convert_luks_create_options(
> > > > RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > > rbd_encryption_algorithm_t *alg,
> > > > - char **passphrase,
> > > > + const char **passphrase,
> > > > size_t *passphrase_len,
> > > > Error **errp)
> > > > {
> > > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > > Error **errp)
> > > > {
> > > > int r = 0;
> > > > - g_autofree char *passphrase = NULL;
> > > > - size_t passphrase_len;
> > > > + g_autofree const char *passphrase = NULL;
> > >
> > > This looks wierd. If it is as const string, why are
> > > we free'ing it ? Either want g_autofree, or const,
> > > but not both.
> >
> > Just curious, is it a requirement imposed by g_autofree? Otherwise
> > pointer constness and pointee lifetime are completely orthogonal and
> > freeing (or, in this case, wanting to auto-free) an object referred to
> > by a const pointer seems perfectly fine to me.
>
> Free'ing a const point is not OK
>
> $ cat c.c
> #include <stdlib.h>
> void bar(const char *foo) {
> free(foo);
> }
>
> $ gcc -Wall -c c.c
> c.c: In function ‘bar’:
> c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> 5 | free(foo);
> | ^~~
> In file included from c.c:2:
> /usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
> 568 | extern void free (void *__ptr) __THROW;
> | ~~~~~~^~~~~
>
> The g_autofree happens to end up hiding this warning, because the const
> annotation isn't propagated to the registere callback, but that doesn't
> mean we should do that.
>
> When a programmer sees a variable annotated const, they expect that
> either someone else is responsible for free'ing it, or that the data
> is statically initialized or stack allocated and thus doesn't need
> free'ing. So g_autofree + const is just wrong.
FWIW many believe that this specification of free() was a mistake and
that it should have been specified to take const void *. Some projects
actually went ahead and fixed that: kfree() and friends in the Linux
kernel take const void *, for example. C++ delete operator works on
const pointers as well -- because object creation and destruction is
fundamentally independent of modification.
But this is more of a philosophical thing... I asked about g_autofree
because a quick grep revealed a bunch of g_autofree const char * locals
in the tree. Or would probably prefer to just drop const here ;)
Thanks,
Ilya
On Thu, Jan 12, 2023 at 06:07:58PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2023 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> > > On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > > > Add const modifier to passphrases,
> > > > > and remove redundant stack variable passphrase_len.
> > > > >
> > > > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > > > ---
> > > > > block/rbd.c | 24 ++++++++++--------------
> > > > > 1 file changed, 10 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > > index f826410f40..e575105e6d 100644
> > > > > --- a/block/rbd.c
> > > > > +++ b/block/rbd.c
> > > > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > > > #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > > > static int qemu_rbd_convert_luks_options(
> > > > > RbdEncryptionOptionsLUKSBase *luks_opts,
> > > > > - char **passphrase,
> > > > > + const char **passphrase,
> > > > > size_t *passphrase_len,
> > > > > Error **errp)
> > > > > {
> > > > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > > > static int qemu_rbd_convert_luks_create_options(
> > > > > RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > > > rbd_encryption_algorithm_t *alg,
> > > > > - char **passphrase,
> > > > > + const char **passphrase,
> > > > > size_t *passphrase_len,
> > > > > Error **errp)
> > > > > {
> > > > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > > > Error **errp)
> > > > > {
> > > > > int r = 0;
> > > > > - g_autofree char *passphrase = NULL;
> > > > > - size_t passphrase_len;
> > > > > + g_autofree const char *passphrase = NULL;
> > > >
> > > > This looks wierd. If it is as const string, why are
> > > > we free'ing it ? Either want g_autofree, or const,
> > > > but not both.
> > >
> > > Just curious, is it a requirement imposed by g_autofree? Otherwise
> > > pointer constness and pointee lifetime are completely orthogonal and
> > > freeing (or, in this case, wanting to auto-free) an object referred to
> > > by a const pointer seems perfectly fine to me.
> >
> > Free'ing a const point is not OK
> >
> > $ cat c.c
> > #include <stdlib.h>
> > void bar(const char *foo) {
> > free(foo);
> > }
> >
> > $ gcc -Wall -c c.c
> > c.c: In function ‘bar’:
> > c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> > 5 | free(foo);
> > | ^~~
> > In file included from c.c:2:
> > /usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
> > 568 | extern void free (void *__ptr) __THROW;
> > | ~~~~~~^~~~~
> >
> > The g_autofree happens to end up hiding this warning, because the const
> > annotation isn't propagated to the registere callback, but that doesn't
> > mean we should do that.
> >
> > When a programmer sees a variable annotated const, they expect that
> > either someone else is responsible for free'ing it, or that the data
> > is statically initialized or stack allocated and thus doesn't need
> > free'ing. So g_autofree + const is just wrong.
>
> FWIW many believe that this specification of free() was a mistake and
> that it should have been specified to take const void *. Some projects
> actually went ahead and fixed that: kfree() and friends in the Linux
> kernel take const void *, for example. C++ delete operator works on
> const pointers as well -- because object creation and destruction is
> fundamentally independent of modification.
I'd really not like that as IMHO seeing the 'const' gives an important
hint to developers as to who is responsible for the releasing the pointer
> But this is more of a philosophical thing... I asked about g_autofree
> because a quick grep revealed a bunch of g_autofree const char * locals
> in the tree. Or would probably prefer to just drop const here ;)
IMHO those existing cases are all bugs that we should fix, along with
adding a rule to checkpatch.pl to detect this mistake.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2026 Red Hat, Inc.