From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrQ69-0006av-Mm for qemu-devel@nongnu.org; Wed, 18 Jul 2012 04:59:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SrQ63-0006ZB-MT for qemu-devel@nongnu.org; Wed, 18 Jul 2012 04:59:09 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:55224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SrQ63-0006Z7-BL for qemu-devel@nongnu.org; Wed, 18 Jul 2012 04:59:03 -0400 Received: by lbok6 with SMTP id k6so2045299lbo.4 for ; Wed, 18 Jul 2012 01:59:02 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <5005C643.5000606@linux.vnet.ibm.com> References: <1341307259-27262-1-git-send-email-harsh@linux.vnet.ibm.com> <1341307259-27262-3-git-send-email-harsh@linux.vnet.ibm.com> <5005B6A5.7030000@linux.vnet.ibm.com> <5005C643.5000606@linux.vnet.ibm.com> Date: Wed, 18 Jul 2012 09:59:02 +0100 Message-ID: From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Subject: Re: [Qemu-devel] [PATCH v7 2/3] Simpletrace v2: Support multiple arguments, strings. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Harsh Bora Cc: qemu-devel@nongnu.org, stefanha@linux.vnet.ibm.com On Tue, Jul 17, 2012 at 9:08 PM, Harsh Bora 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 >>> 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