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é <berra...@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 <o...@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 :|
