Patchwork srp: Don't use QEMU_PACKED for single elements of a structured type

login
register
mail settings
Submitter Stefan Weil
Date Aug. 10, 2012, 8:03 p.m.
Message ID <1344629007-23311-3-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/176621/
State Accepted
Headers show

Comments

Stefan Weil - Aug. 10, 2012, 8:03 p.m.
QEMU_PACKED results in a MinGW compiler warning when it is
used for single structure elements:

warning: 'gcc_struct' attribute ignored

Using QEMU_PACKED for the whole structure avoids the compiler warning
without changing the memory layout.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 hw/srp.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)
Stefan Hajnoczi - Aug. 15, 2012, 2:16 p.m.
On Fri, Aug 10, 2012 at 10:03:27PM +0200, Stefan Weil wrote:
> QEMU_PACKED results in a MinGW compiler warning when it is
> used for single structure elements:
> 
> warning: 'gcc_struct' attribute ignored
> 
> Using QEMU_PACKED for the whole structure avoids the compiler warning
> without changing the memory layout.

Quick link for other reviewers:
http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Type-Attributes.html#Type-Attributes

> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  hw/srp.h |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/srp.h b/hw/srp.h
> index 3009bd5..5e0cad5 100644
> --- a/hw/srp.h
> +++ b/hw/srp.h
> @@ -177,13 +177,13 @@ struct srp_tsk_mgmt {
>      uint8_t    reserved1[6];
>      uint64_t   tag;
>      uint8_t    reserved2[4];
> -    uint64_t   lun QEMU_PACKED;
> +    uint64_t   lun;
>      uint8_t    reserved3[2];
>      uint8_t    tsk_mgmt_func;
>      uint8_t    reserved4;
>      uint64_t   task_tag;
>      uint8_t    reserved5[8];
> -};
> +} QEMU_PACKED;

Here I actually see a difference for the uint64_t task_tag field.
Previously it was not packed, now it is packed and because it has 4 *
uint8_t before it there will be a difference in layout.

Looking at how QEMU accesses srp_tsk_mgmt, I think we're safe because we
never actually access task_tag?

Ben: Any thoughts on this patch?

Stefan
Stefan Weil - Aug. 15, 2012, 3:59 p.m.
Am 15.08.2012 16:16, schrieb Stefan Hajnoczi:
> On Fri, Aug 10, 2012 at 10:03:27PM +0200, Stefan Weil wrote:
>> QEMU_PACKED results in a MinGW compiler warning when it is
>> used for single structure elements:
>>
>> warning: 'gcc_struct' attribute ignored
>>
>> Using QEMU_PACKED for the whole structure avoids the compiler warning
>> without changing the memory layout.
> Quick link for other reviewers:
> http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Type-Attributes.html#Type-Attributes
>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>   hw/srp.h |    8 ++++----
>>   1 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/srp.h b/hw/srp.h
>> index 3009bd5..5e0cad5 100644
>> --- a/hw/srp.h
>> +++ b/hw/srp.h
>> @@ -177,13 +177,13 @@ struct srp_tsk_mgmt {
>>       uint8_t    reserved1[6];
>>       uint64_t   tag;
>>       uint8_t    reserved2[4];
>> -    uint64_t   lun QEMU_PACKED;
>> +    uint64_t   lun;
>>       uint8_t    reserved3[2];
>>       uint8_t    tsk_mgmt_func;
>>       uint8_t    reserved4;
>>       uint64_t   task_tag;
>>       uint8_t    reserved5[8];
>> -};
>> +} QEMU_PACKED;
> Here I actually see a difference for the uint64_t task_tag field.
> Previously it was not packed, now it is packed and because it has 4 *
> uint8_t before it there will be a difference in layout.
>
> Looking at how QEMU accesses srp_tsk_mgmt, I think we're safe because we
> never actually access task_tag?
>
> Ben: Any thoughts on this patch?
>
> Stefan

4 * uint8_t + 4 bytes from the packed lun, so there is no change
for task_tag, it's always on a 8 byte boundary!

Regards,
Stefan
Stefan Hajnoczi - Aug. 16, 2012, 6:53 a.m.
On Wed, Aug 15, 2012 at 05:59:26PM +0200, Stefan Weil wrote:
> Am 15.08.2012 16:16, schrieb Stefan Hajnoczi:
> >On Fri, Aug 10, 2012 at 10:03:27PM +0200, Stefan Weil wrote:
> >>QEMU_PACKED results in a MinGW compiler warning when it is
> >>used for single structure elements:
> >>
> >>warning: 'gcc_struct' attribute ignored
> >>
> >>Using QEMU_PACKED for the whole structure avoids the compiler warning
> >>without changing the memory layout.
> >Quick link for other reviewers:
> >http://gcc.gnu.org/onlinedocs/gcc-4.7.1/gcc/Type-Attributes.html#Type-Attributes
> >
> >>Signed-off-by: Stefan Weil <sw@weilnetz.de>
> >>---
> >>  hw/srp.h |    8 ++++----
> >>  1 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/hw/srp.h b/hw/srp.h
> >>index 3009bd5..5e0cad5 100644
> >>--- a/hw/srp.h
> >>+++ b/hw/srp.h
> >>@@ -177,13 +177,13 @@ struct srp_tsk_mgmt {
> >>      uint8_t    reserved1[6];
> >>      uint64_t   tag;
> >>      uint8_t    reserved2[4];
> >>-    uint64_t   lun QEMU_PACKED;
> >>+    uint64_t   lun;
> >>      uint8_t    reserved3[2];
> >>      uint8_t    tsk_mgmt_func;
> >>      uint8_t    reserved4;
> >>      uint64_t   task_tag;
> >>      uint8_t    reserved5[8];
> >>-};
> >>+} QEMU_PACKED;
> >Here I actually see a difference for the uint64_t task_tag field.
> >Previously it was not packed, now it is packed and because it has 4 *
> >uint8_t before it there will be a difference in layout.
> >
> >Looking at how QEMU accesses srp_tsk_mgmt, I think we're safe because we
> >never actually access task_tag?
> >
> >Ben: Any thoughts on this patch?
> >
> >Stefan
> 
> 4 * uint8_t + 4 bytes from the packed lun, so there is no change
> for task_tag, it's always on a 8 byte boundary!

Ah, yes, I see it now!  Glad we're switching to struct-level packing :).

Stefan

Patch

diff --git a/hw/srp.h b/hw/srp.h
index 3009bd5..5e0cad5 100644
--- a/hw/srp.h
+++ b/hw/srp.h
@@ -177,13 +177,13 @@  struct srp_tsk_mgmt {
     uint8_t    reserved1[6];
     uint64_t   tag;
     uint8_t    reserved2[4];
-    uint64_t   lun QEMU_PACKED;
+    uint64_t   lun;
     uint8_t    reserved3[2];
     uint8_t    tsk_mgmt_func;
     uint8_t    reserved4;
     uint64_t   task_tag;
     uint8_t    reserved5[8];
-};
+} QEMU_PACKED;
 
 /*
  * We need the packed attribute because the SRP spec only aligns the
@@ -198,14 +198,14 @@  struct srp_cmd {
     uint8_t    data_in_desc_cnt;
     uint64_t   tag;
     uint8_t    reserved2[4];
-    uint64_t   lun QEMU_PACKED;
+    uint64_t   lun;
     uint8_t    reserved3;
     uint8_t    task_attr;
     uint8_t    reserved4;
     uint8_t    add_cdb_len;
     uint8_t    cdb[16];
     uint8_t    add_data[0];
-};
+} QEMU_PACKED;
 
 enum {
     SRP_RSP_FLAG_RSPVALID = 1 << 0,