[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] trace: Fix simple trace dropped event recor
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] trace: Fix simple trace dropped event record for big endian |
Date: |
Thu, 31 Jan 2013 13:39:09 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130108 Thunderbird/10.0.12 |
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