Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings.

[Stefan Hajnoczi]

On Tue, Jul 17, 2012 at 9:08 PM, Harsh Bora <address@hidden> wrote:
> On 07/18/2012 12:31 AM, Harsh Bora wrote:
>>
>> On 07/17/2012 08:51 PM, Stefan Hajnoczi wrote:
>>>
>>> On Tue, Jul 3, 2012 at 10:20 AM, Harsh Prateek Bora
>>> <address@hidden> wrote:
>>>> @@ -75,16 +96,31 @@ static char *trace_file_name = NULL;
>>>>    *
>>>>    * Returns false if the record is not valid.
>>>>    */
>>>> -static bool get_trace_record(unsigned int idx, TraceRecord *record)
>>>> +static bool get_trace_record(unsigned int idx, TraceRecord **recordptr)
>>>>   {
>>>> -    if (!(trace_buf[idx].event & TRACE_RECORD_VALID)) {
>>>> +    uint8_t rec_hdr[sizeof(TraceRecord)];
>>>> +    uint64_t event_flag = 0;
>>>> +    TraceRecord *record = (TraceRecord *) rec_hdr;
>>>
>>>
>>> Declaring rec_hdr as a uint8_t array is only because you're trying to
>>> avoid a cast later?  The easiest way to make this nice is to do what
>>> memset(), memcpy() and friends do: just use a void *buf argument.
>>> That way a caller can pass a TraceRecord* or any other pointer without
>>> explicit casts, unions, or the uint8_t array trick you are using.
>>
>>
>> Are you suggesting to use malloc() here?
>>
>> void *rec_hdr = malloc(sizeof(TraceRecord));
>>
>> I kept a static array to make sure structure padding doesnt take place.
>> I am not sure if using malloc here is recommended as we are reading
>> header and then writing this header byte-by-byte?
>
>
> Ah, I confused it with trace_record_finish where we write back the
> previously read header, which is not the case here. However, I still feel
> using an array is better here probably because:
> 1) We anyway cant use memcpy here to read from global buffer, we have to use
> read_from_buffer to take care of buffer boundaries.
> 2) Isnt malloc() expensive for such a small allocation requirement?

No malloc.

The code is basically playing games with types because the read/write
functions take a uint8_t* buffer argument but callers actually want to
use TraceRecord.  You ended up declaring a uint8_t array and then
pointing a TraceRecord* into the array.

A cleaner way of doing this is to just use TraceRecord and make the
read/write functions take void* buffer arguments.  My comment about
memset/memcpy was that these library functions do the same thing - it
allows callers to write clean and simple code.

You can drop the uint8_t arrays and TraceRecord* alias pointers.  You
can also drop the union you have in one of the functions.

Just use a TraceRecord local variable and change the read/write helper
buffer argument to void*.

Stefan