On Tue, Jul 17, 2012 at 9:08 PM, Harsh Bora <ha...@linux.vnet.ibm.com> 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
>>> <ha...@linux.vnet.ibm.com> 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
