Re: [Qemu-devel] [PATCH 1/4] trace: Fix simple trace dropped event record for big endian

[Laszlo Ersek]

On 01/31/13 13:10, Markus Armbruster wrote:
> Laszlo Ersek <address@hidden> writes:
>> On 01/25/13 16:43, Markus Armbruster wrote:

>>> @@ -63,7 +63,7 @@ typedef struct {
>>>      uint64_t timestamp_ns;
>>>      uint32_t length;   /*    in bytes */
>>>      uint32_t reserved; /*    unused */
>>> -    uint8_t arguments[];
>>> +    uint64_t arguments[];
>>>  } TraceRecord;
>>>  
>>>  typedef struct {
>>> @@ -160,7 +160,7 @@ static gpointer writeout_thread(gpointer opaque)
>>>          uint8_t bytes[sizeof(TraceRecord) + sizeof(uint64_t)];
>>>      } dropped;
>>>      unsigned int idx = 0;
>>> -    uint64_t dropped_count;
>>> +    int dropped_count;
>>>      size_t unused __attribute__ ((unused));
>>>  
>>>      for (;;) {
>>> @@ -169,16 +169,16 @@ static gpointer writeout_thread(gpointer opaque)
>>>          if (dropped_events) {
>>>              dropped.rec.event = DROPPED_EVENT_ID,
>>>              dropped.rec.timestamp_ns = get_clock();
>>> -            dropped.rec.length = sizeof(TraceRecord) + 
>>> sizeof(dropped_events),
>>> +            dropped.rec.length = sizeof(TraceRecord) + sizeof(uint64_t),
>>>              dropped.rec.reserved = 0;
>>>              while (1) {
>>>                  dropped_count = dropped_events;
>>> -                if (g_atomic_int_compare_and_exchange((gint 
>>> *)&dropped_events,
>>> +                if (g_atomic_int_compare_and_exchange(&dropped_events,
>>>                                                        dropped_count, 0)) {
>>>                      break;
>>>                  }
>>>              }
>>> -            memcpy(dropped.rec.arguments, &dropped_count, 
>>> sizeof(uint64_t));
>>> +            dropped.rec.arguments[0] = dropped_count;
>>
>> I *think* I'd prefer if the element type of TraceRecord.arguments[] were
>> not changed; an array of u8's seems to be more flexible. The element
>> type change appears both unrelated and not really necessary for this
>> patch. (You'd have to keep the memcpy(), but it doesn't hurt.)
> 
> Casting uint64_t[] to uint8_t is safe.  Casting uint8_t[] to FOO * isn't
> when alignof(FOO) > 1.  That's why I really don't like uint8_t[] there.

The thought of casting &arguments[N] to another pointer type didn't
cross my mind (independently of the element type). Who would do such a
thing? memcpy() is pretty much a requirement in this case, in my world.

> It happens to be amply aligned, and it happens to be used only with
> memcpy().  But neither is immediately obvious, or obviously bound to
> stay that way.

Changing the element type to uint64_t doesn't guarantee in general that
(type*)&arguments[N] will be a correctly aligned pointer. (The
guaranteed cases are when "type" is a character type, void, or uint64_t).

Anyway my reviewed-by stands.

Laszlo