Message ID | 1400163717-1898-2-git-send-email-kwolf@redhat.com |
---|---|
State | New |
Headers | show |
On 05/15/2014 08:21 AM, Kevin Wolf wrote: > We were relying on all compilers inserting the same padding in the > header struct that is used for the on-disk format. Let's not do that. > Mark the struct as packed and insert an explicit padding field for > compatibility. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: Benoit Canet <benoit@irqsave.net> > --- > block/qcow.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/block/qcow.c b/block/qcow.c > index 937dd6d..3684794 100644 > --- a/block/qcow.c > +++ b/block/qcow.c > @@ -48,9 +48,10 @@ typedef struct QCowHeader { > uint64_t size; /* in bytes */ > uint8_t cluster_bits; > uint8_t l2_bits; > + uint16_t padding; > uint32_t crypt_method; > uint64_t l1_table_offset; > -} QCowHeader; > +} QEMU_PACKED QCowHeader; Is it worth a compile-time assertion that the correct size is achieved? [I don't know if glib provides such a macro, but gnulib has a verify() macro that could be used as: verify(sizeof(QCowHeader) == NNN) which expands to _Static_assert(sizeof(QCowHeader) == NNN) in new enough C compilers, and to something like extern int (*dummy1(void)) [sizeof (struct dummy2 { int dummy3: (sizeof(QCowHeader) == NNN) ? 1 : -1; })] on older compilers for reliable compile-time detection] But not a show-stopper to this patch as-is.
Am 15.05.2014 um 17:49 hat Eric Blake geschrieben: > On 05/15/2014 08:21 AM, Kevin Wolf wrote: > > We were relying on all compilers inserting the same padding in the > > header struct that is used for the on-disk format. Let's not do that. > > Mark the struct as packed and insert an explicit padding field for > > compatibility. > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > Reviewed-by: Benoit Canet <benoit@irqsave.net> > > --- > > block/qcow.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/block/qcow.c b/block/qcow.c > > index 937dd6d..3684794 100644 > > --- a/block/qcow.c > > +++ b/block/qcow.c > > @@ -48,9 +48,10 @@ typedef struct QCowHeader { > > uint64_t size; /* in bytes */ > > uint8_t cluster_bits; > > uint8_t l2_bits; > > + uint16_t padding; > > uint32_t crypt_method; > > uint64_t l1_table_offset; > > -} QCowHeader; > > +} QEMU_PACKED QCowHeader; > > Is it worth a compile-time assertion that the correct size is achieved? > > [I don't know if glib provides such a macro, but gnulib has a verify() > macro that could be used as: > > verify(sizeof(QCowHeader) == NNN) > > which expands to _Static_assert(sizeof(QCowHeader) == NNN) in new enough > C compilers, and to something like > > extern int (*dummy1(void)) [sizeof (struct dummy2 { > int dummy3: (sizeof(QCowHeader) == NNN) ? 1 : -1; })] > > on older compilers for reliable compile-time detection] > > But not a show-stopper to this patch as-is. QEMU_BUILD_BUG_ON() is what you're looking for. Do you think that would be a useful addition? With packed structs there should be little that could make it go wrong. But if we want to add this, I'd do it in a separate patch and for all image formats. Kevin
On 05/16/2014 04:41 AM, Kevin Wolf wrote: >>> +} QEMU_PACKED QCowHeader; >> >> Is it worth a compile-time assertion that the correct size is achieved? >> > > QEMU_BUILD_BUG_ON() is what you're looking for. Ah, thanks. > > Do you think that would be a useful addition? With packed structs there > should be little that could make it go wrong. But if we want to add > this, I'd do it in a separate patch and for all image formats. Definitely a separate patch. But yes, I think that it can't hurt - even with packed structs, it's a bit of insurance against someone accidentally adding a field or using a wrong type, particularly for any packed structs where a newer version converts reserved space into a named purpose.
diff --git a/block/qcow.c b/block/qcow.c index 937dd6d..3684794 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -48,9 +48,10 @@ typedef struct QCowHeader { uint64_t size; /* in bytes */ uint8_t cluster_bits; uint8_t l2_bits; + uint16_t padding; uint32_t crypt_method; uint64_t l1_table_offset; -} QCowHeader; +} QEMU_PACKED QCowHeader; #define L2_CACHE_SIZE 16