mbox

[PULL,00/11] Ide patches

Message ID 1452300718-10660-1-git-send-email-jsnow@redhat.com
State New
Headers show

Pull-request

https://github.com/jnsnow/qemu.git tags/ide-pull-request

Message

John Snow Jan. 9, 2016, 12:51 a.m. UTC 
The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:

  Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)

are available in the git repository at:

  https://github.com/jnsnow/qemu.git tags/ide-pull-request

for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:

  libqos/ahci: organize header (2016-01-08 15:22:34 -0500)

----------------------------------------------------------------

----------------------------------------------------------------

John Snow (9):
  ahci-test: fix memory leak
  libqos/ahci: ATAPI support
  libqos/ahci: ATAPI identify
  libqos/ahci: Switch to mutable properties
  libqos: allow zero-size allocations
  libqos/ahci: allow nondata commands for ahci_io variants
  libqos/ahci: add ahci_exec
  qtest/ahci: ATAPI data tests
  libqos/ahci: organize header

Mark Cave-Ayland (1):
  macio: fix overflow in lba to offset conversion for ATAPI devices

Prasad J Pandit (1):
  ide: ahci: reset ncq object to unused on error

 hw/ide/ahci.c         |   1 +
 hw/ide/macio.c        |   2 +-
 tests/ahci-test.c     | 131 ++++++++++++++++++++++++++++++------
 tests/libqos/ahci.c   | 181 +++++++++++++++++++++++++++++++++++++++++++++++---
 tests/libqos/ahci.h   |  66 +++++++++++++++---
 tests/libqos/malloc.c |   4 ++
 6 files changed, 343 insertions(+), 42 deletions(-)

Comments

Peter Maydell Jan. 11, 2016, 11:18 a.m. UTC | #1 
On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
> The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:
>
>   Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)
>
> are available in the git repository at:
>
>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>
> for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:
>
>   libqos/ahci: organize header (2016-01-08 15:22:34 -0500)
>
> ----------------------------------------------------------------
>
> ----------------------------------------------------------------

These seem to result in some new clang sanitizer runtime warnings
during a 'make check':

/home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:963:9:
runtime error: store to misaligned address 0x2adacfbaacd7 for type
'uint16_t' (aka 'unsigned short'), which requires 2 byte alignment
0x2adacfbaacd7: note: pointer points here
 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  21
00 00 00 00 00 00 00  6c 6f 6e
             ^
/home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:907:9:
runtime error: store to misaligned address 0x2adacfbaacd2 for type
'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
0x2adacfbaacd2: note: pointer points here
 00 00  28 00 00 00 00 00 00 00  01 00 00 00 00 00 00 00  00 00 00 00
00 00 00 00  21 00 00 00 00 00
              ^

This kind of thing:

    unsigned char *cbd = cmd->atapi_cmd;
    uint32_t *lba32;

        lba32 = (uint32_t *)&(cbd[2]);
        *lba32 = cpu_to_be32(lba);

isn't valid. You probably want
 stl_be_p(&cbd[2], lba);

(defined in qemu/bswap.h).

thanks
-- PMM
John Snow Jan. 11, 2016, 5:18 p.m. UTC | #2 
On 01/11/2016 06:18 AM, Peter Maydell wrote:
> On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
>> The following changes since commit 38a762fec63fd5c035aae29ba9a77d357e21e4a7:
>>
>>   Merge remote-tracking branch 'remotes/berrange/tags/pull-crypto-fixes-2015-12-23-1' into staging (2015-12-23 13:53:32 +0000)
>>
>> are available in the git repository at:
>>
>>   https://github.com/jnsnow/qemu.git tags/ide-pull-request
>>
>> for you to fetch changes up to 4160ad843841df21de296016fb77f986e693bed2:
>>
>>   libqos/ahci: organize header (2016-01-08 15:22:34 -0500)
>>
>> ----------------------------------------------------------------
>>
>> ----------------------------------------------------------------
> 
> These seem to result in some new clang sanitizer runtime warnings
> during a 'make check':
> 
> /home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:963:9:
> runtime error: store to misaligned address 0x2adacfbaacd7 for type
> 'uint16_t' (aka 'unsigned short'), which requires 2 byte alignment
> 0x2adacfbaacd7: note: pointer points here
>  00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  21
> 00 00 00 00 00 00 00  6c 6f 6e
>              ^
> /home/petmay01/linaro/qemu-for-merges/tests/libqos/ahci.c:907:9:
> runtime error: store to misaligned address 0x2adacfbaacd2 for type
> 'uint32_t' (aka 'unsigned int'), which requires 4 byte alignment
> 0x2adacfbaacd2: note: pointer points here
>  00 00  28 00 00 00 00 00 00 00  01 00 00 00 00 00 00 00  00 00 00 00
> 00 00 00 00  21 00 00 00 00 00
>               ^
> 
> This kind of thing:
> 

"This kind of thing" as one might say while holding up a rotting fish
with just two fingers, held at arm's length.

>     unsigned char *cbd = cmd->atapi_cmd;
>     uint32_t *lba32;
> 
>         lba32 = (uint32_t *)&(cbd[2]);
>         *lba32 = cpu_to_be32(lba);
> 
> isn't valid. You probably want
>  stl_be_p(&cbd[2], lba);
> 
> (defined in qemu/bswap.h).
> 
> thanks
> -- PMM
> 

Thanks for the pointer. Out of curiosity, is there no standard way to
perform this kind of operation in C? I want to adjust my bad habits. In
QEMU I can remember to use these macros now that I know they're there,
but not sure what I'd use in other projects. memcpy directly?

--js
Peter Maydell Jan. 11, 2016, 5:36 p.m. UTC | #3 
On 11 January 2016 at 17:18, John Snow <jsnow@redhat.com> wrote:
> On 01/11/2016 06:18 AM, Peter Maydell wrote:
>> On 9 January 2016 at 00:51, John Snow <jsnow@redhat.com> wrote:
>> This kind of thing:
>>
>
> "This kind of thing" as one might say while holding up a rotting fish
> with just two fingers, held at arm's length.

Not the intended tone :-)

>>     unsigned char *cbd = cmd->atapi_cmd;
>>     uint32_t *lba32;
>>
>>         lba32 = (uint32_t *)&(cbd[2]);
>>         *lba32 = cpu_to_be32(lba);
>>
>> isn't valid. You probably want
>>  stl_be_p(&cbd[2], lba);

> Thanks for the pointer. Out of curiosity, is there no standard way to
> perform this kind of operation in C? I want to adjust my bad habits. In
> QEMU I can remember to use these macros now that I know they're there,
> but not sure what I'd use in other projects. memcpy directly?

You can use memcpy, or you can hand-assemble values in and out
of byte arrays, I think. memcpy() is generally recommended, because
the compiler does a decent job with it.

thanks
-- PMM