qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 5/9] IDE: replace DEBUG_AIO with


From: Laszlo Ersek
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 5/9] IDE: replace DEBUG_AIO with trace events
Date: Thu, 31 Aug 2017 18:19:01 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

On 08/31/17 01:25, John Snow wrote:
> CCing Laszlo Ersek literally just for laughs, as he is the most
> entertaining language lawyer I know of.
> 
> Laszlo, please feel free to ignore this if you don't care :P
> 
> On 08/29/2017 11:14 PM, Philippe Mathieu-Daudé wrote:
>> Hi John,
>>
>> On 08/29/2017 05:49 PM, John Snow wrote:
>>> Signed-off-by: John Snow <address@hidden>
>>> ---
>>>   hw/ide/atapi.c            |  5 +----
>>>   hw/ide/core.c             | 17 ++++++++++-------
>>>   hw/ide/trace-events       |  3 +++
>>>   include/hw/ide/internal.h |  6 ++++--
>>>   4 files changed, 18 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>> index 37fa699..b8fc51e 100644
>>> --- a/hw/ide/atapi.c
>>> +++ b/hw/ide/atapi.c
>>> @@ -416,10 +416,7 @@ static void ide_atapi_cmd_read_dma_cb(void
>>> *opaque, int ret)
>>>           s->io_buffer_size = n * 2048;
>>>           data_offset = 0;
>>>       }
>>> -#ifdef DEBUG_AIO
>>> -    printf("aio_read_cd: lba=%u n=%d\n", s->lba, n);
>>> -#endif
>>> -
>>> +    trace_ide_atapi_cmd_read_dma_cb_aio(s, s->lba, n);
>>>       s->bus->dma->iov.iov_base = (void *)(s->io_buffer + data_offset);
>>>       s->bus->dma->iov.iov_len = n * ATAPI_SECTOR_SIZE;
>>>       qemu_iovec_init_external(&s->bus->dma->qiov, &s->bus->dma->iov, 1);
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 82a19b1..a1c90e9 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -58,6 +58,13 @@ static const int smart_attributes[][12] = {
>>>       { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00,
>>> 0x00, 0x32},
>>>   };
>>>   +const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT] = {
>>> +    [IDE_DMA_READ] = "DMA READ",
>>> +    [IDE_DMA_WRITE] = "DMA WRITE",
>>> +    [IDE_DMA_TRIM] = "DMA TRIM",
>>> +    [IDE_DMA_ATAPI] = "DMA ATAPI"
>>> +};
>>> +
>>>   static void ide_dummy_transfer_stop(IDEState *s);
>>>     static void padstr(char *str, const char *src, int len)
>>> @@ -860,10 +867,8 @@ static void ide_dma_cb(void *opaque, int ret)
>>>           goto eot;
>>>       }
>>>   -#ifdef DEBUG_AIO
>>> -    printf("ide_dma_cb: sector_num=%" PRId64 " n=%d, cmd_cmd=%d\n",
>>> -           sector_num, n, s->dma_cmd);
>>> -#endif
>>> +    trace_ide_dma_cb(s, sector_num, n,
>>> +                     IDE_DMA_CMD_lookup[s->dma_cmd]);
>>>         if ((s->dma_cmd == IDE_DMA_READ || s->dma_cmd ==
>>> IDE_DMA_WRITE) &&
>>>           !ide_sect_range_ok(s, sector_num, n)) {
>>> @@ -2391,9 +2396,7 @@ void ide_bus_reset(IDEBus *bus)
>>>         /* pending async DMA */
>>>       if (bus->dma->aiocb) {
>>> -#ifdef DEBUG_AIO
>>> -        printf("aio_cancel\n");
>>> -#endif
>>> +        trace_ide_bus_reset_aio();
>>>           blk_aio_cancel(bus->dma->aiocb);
>>>           bus->dma->aiocb = NULL;
>>>       }
>>> diff --git a/hw/ide/trace-events b/hw/ide/trace-events
>>> index 8c79a6c..cc8949c 100644
>>> --- a/hw/ide/trace-events
>>> +++ b/hw/ide/trace-events
>>> @@ -18,6 +18,8 @@ ide_cancel_dma_sync_remaining(void) "draining all
>>> remaining requests"
>>>   ide_sector_read(int64_t sector_num, int nsectors) "sector=%"PRId64"
>>> nsectors=%d"
>>>   ide_sector_write(int64_t sector_num, int nsectors) "sector=%"PRId64"
>>> nsectors=%d"
>>>   ide_reset(void *s) "IDEstate %p"
>>> +ide_bus_reset_aio(void) "aio_cancel"
>>> +ide_dma_cb(void *s, int64_t sector_num, int n, const char *dma)
>>> "IDEState %p; sector_num=%"PRId64" n=%d cmd=%s"
>>>     # BMDMA HBAs:
>>>   @@ -51,5 +53,6 @@ ide_atapi_cmd_reply_end_new(void *s, int status)
>>> "IDEState: %p; new transfer sta
>>>   ide_atapi_cmd_check_status(void *s) "IDEState: %p"
>>>   ide_atapi_cmd_read(void *s, const char *method, int lba, int
>>> nb_sectors) "IDEState: %p; read %s: LBA=%d nb_sectors=%d"
>>>   ide_atapi_cmd(void *s, uint8_t cmd) "IDEState: %p; cmd: 0x%02x"
>>> +ide_atapi_cmd_read_dma_cb_aio(void *s, int lba, int n) "IDEState: %p;
>>> aio read: lba=%d n=%d"
>>>   # Warning: Verbose
>>>   ide_atapi_cmd_packet(void *s, uint16_t limit, const char *packet)
>>> "IDEState: %p; limit=0x%x packet: %s"
>>> diff --git a/include/hw/ide/internal.h b/include/hw/ide/internal.h
>>> index 74efe8a..db9fde0 100644
>>> --- a/include/hw/ide/internal.h
>>> +++ b/include/hw/ide/internal.h
>>> @@ -14,7 +14,6 @@
>>>   #include "block/scsi.h"
>>>     /* debug IDE devices */
>>> -//#define DEBUG_AIO
>>>   #define USE_DMA_CDROM
>>>     typedef struct IDEBus IDEBus;
>>> @@ -333,12 +332,15 @@ struct unreported_events {
>>>   };
>>>     enum ide_dma_cmd {
>>> -    IDE_DMA_READ,
>>> +    IDE_DMA_READ = 0,
>>>       IDE_DMA_WRITE,
>>>       IDE_DMA_TRIM,
>>>       IDE_DMA_ATAPI,
>>> +    IDE_DMA__COUNT
>>>   };
>>>   +extern const char *IDE_DMA_CMD_lookup[IDE_DMA__COUNT];
>>
>> I recommend you to avoid this declaring extern const array with size, I
>> remember some compilers (old GCC?) ignoring array size in extern. Eric
>> will correct me!
>>
>> It is much safer to use a getter:
>>
> 
> Well, whether or not the compiler ignores it, you're right that it's
> safer to use a getter. I don't think the width being declared HURTS any
> compiler though, does it?

If, in the above declaration, you say

extern const char *IDE_DMA_CMD_lookup[];

then the "IDE_DMA_CMD_lookup" will have "incomplete array type". In a
translation unit that includes this declaration, but doesn't actually
*define* the array (thereby completing its type), you can e.g. subscript
the array like this:

  IDE_CMD_DMA_lookup[IDE_DMA_WRITE]

but you cannot do anything with it that would require it to have a
complete type, such as:

  sizeof IDE_CMD_DMA_lookup


If you declare the array as quoted above, then the type will be complete
for all translation units that include the declaration, not just for the
one that defines the array.

Which one I'd recommend depends on the use case. If it's OK to expose
the size of the array to all consumers (or the IDE_DMA__COUNT
enumeration constant), then I'd go with the complete type in the
declaration. It communicates the intent.

And no, the compiler is not required to catch your out-of-bounds access
in either case; but that's not the point. The point is that the human
that looks at the declaration will know how to bounds-check the access.

If you write a getter function, then either declaration will work just
fine, but in that case, you don't even need the array. Just add a switch
statement to your getter, and return the addresses of open-coded string
literals.

> 
>> const char *IDE_DMA_CMD_lookup(enum ide_dma_cmd cmd)
>> {
>>     static const char *IDE_DMA_CMD_name[IDE_DMA__COUNT] = {
>>         [IDE_DMA_READ] = "DMA READ",
>>         [IDE_DMA_WRITE] = "DMA WRITE",
>>         [IDE_DMA_TRIM] = "DMA TRIM",
>>         [IDE_DMA_ATAPI] = "DMA ATAPI"
>>     };
>>
>>     return IDE_DMA_CMD_name[cmd];
>> };
>>
> 
> Why is this safer...?
> 
> Here's my opinion of enums:
> 
> address@hidden ~/s/scratch> cat enum.c
> enum foo {
>     FOO_A = 0,
>     FOO_B
> };
> 
> int fn(enum foo in)
> {
>     switch (in) {
>     case FOO_A:
>     case FOO_B:
>         return 0;
>     default:
>         return 1;
>     }
> }
> 
> int main(int argc, char *argv[])
> {
>     return fn(2);
> }
> 
> 
> address@hidden ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
> address@hidden ~/s/scratch> ./enum
> address@hidden ~/s/scratch> echo $status
> 1
> 
> 
> No warnings, no messages, it just happily takes a value outside the
> domain and in this case it doesn't explode because I caught it, but,
> take a look:
> 
> address@hidden ~/s/scratch> cat enum.c
> #include <stdio.h>
> 
> enum foo {
>     FOO_A = 0,
>     FOO_B,
>     FOO__COUNT
> };
> 
> const char *table[FOO__COUNT] = {"FOO_A", "FOO_B"};
> 
> int fn(enum foo in)
> {
>     fprintf(stderr, "%s\n", table[in]);
>     switch (in) {
>     case FOO_A:
>     case FOO_B:
>         return 0;
>     default:
>         return 1;
>     }
> }
> 
> int main(int argc, char *argv[])
> {
>     return fn(2);
> }
> 
> Similar program, let's compile it:
> 
> address@hidden ~/s/scratch> gcc -ansi -Wall -pedantic -o enum enum.c
> address@hidden ~/s/scratch> ./enum
> � �
> 
> 
> Ah, dang.
> 
> This is why I prefer to have my assertions and don't trust enums to be
> checked rigorously.
> 
> For anyone playing along at home, yes, if you specify at least -O1 here
> it will catch it:
> 
> address@hidden ~/s/scratch> gcc -ansi -Wall -pedantic -O3 -o enum enum.c
> enum.c: In function ‘main’:
> enum.c:13:5: warning: array subscript is above array bounds [-Warray-bounds]
>      fprintf(stderr, "%s\n", table[in]);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Oh no, what the hell is clang up to, though?
> 
> address@hidden ~/s/scratch> clang -ansi -Wall -pedantic -O3 -o enum enum.c
> address@hidden ~/s/scratch>
> 
> 
> Oh no...
> 
> http://i.imgur.com/uRvorq3.jpg

I wouldn't like to quote all the standardese related to enums. My point
here is that the compiler might choose "signed char", for example, for
representing the "enum foo" type. Let's say the range of "signed char"
is -128..127 inclusive.

Now assume you pass value 128 (of type "int") to a function that takes
"enum foo", such as your fn(). The argument is converted as in an
assignment, and the following applies:

  6.3 Conversions
  6.3.1 Arithmetic operands
  6.3.1.1 Boolean, characters, and integers
    1 [...]
       - The rank of any enumerated type shall equal the rank of the
         compatible integer type (see 6.7.2.2).
    [...]
  6.3.1.3 Signed and unsigned integers
    [...]
    3 Otherwise, the new type is signed and the value cannot be
      represented in it; either the result is implementation-defined or
      an implementation-defined signal is raised.

Dependent on what the docs for your C language implementation say, you
could get total garbage in fn(), or you could get a signal before your
code in fn() is reached.

IOW, if you cannot guarantee *before* the function call that the value
you pass to fn() -- or convert to "enum foo" otherwise -- will nicely
fit in "enum foo"'s represenative integer type, then catching it in fn()
is too late.

I suggest assigning to "enum foo" only
- another "enum foo" object,
- a matching enumeration constant (which has type "int", but will fit of
  course),
- any integer value that has been range-checked explicitly.

The last point does encourage that your getter function should take,
say, "unsigned int" rather than "enum foo"; it handles incorrect calls
more portably.

... I'm unsure if this wall of text is relevant *at all* here, but I've
had to live up to past entertainment! ;)

Laszlo

> 
>> If you agree:
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>>
> 
> Somewhat. I will write a getter that does bounds checking. It _is_ safer
> that way, though I don't think we'll ever run into a situation where it
> would come up.
> 
>>> +
>>>   #define ide_cmd_is_read(s) \
>>>       ((s)->dma_cmd == IDE_DMA_READ)
>>>  
>>
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]