| Message ID | 1344629007-23311-3-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
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
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
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
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,
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(-)