From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=52542 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PwabT-0001G8-St for qemu-devel@nongnu.org; Mon, 07 Mar 2011 08:36:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PwabS-00074l-Ez for qemu-devel@nongnu.org; Mon, 07 Mar 2011 08:36:03 -0500 Received: from mail-yi0-f45.google.com ([209.85.218.45]:57204) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PwabS-00074V-BK for qemu-devel@nongnu.org; Mon, 07 Mar 2011 08:36:02 -0500 Received: by yib19 with SMTP id 19so2015457yib.4 for ; Mon, 07 Mar 2011 05:36:01 -0800 (PST) Message-ID: <4D74DF40.1070100@codemonkey.ws> Date: Mon, 07 Mar 2011 07:36:00 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 03/22] qapi: add Error object References: <1299460984-15849-1-git-send-email-aliguori@us.ibm.com> <1299460984-15849-4-git-send-email-aliguori@us.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Adam Litke , Markus Armbruster , Luiz Capitulino On 03/07/2011 05:38 AM, Stefan Hajnoczi wrote: > On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori wrote: > >> +struct Error >> +{ >> + QDict *obj; >> + const char *fmt; >> + char *msg; >> +}; >> > I wonder why fmt is const char * but msg is char *. Users should use > error_get_pretty() instead of accessing msg directly and that function > returns const char * so it seems that msg should be const char * to > start with. > fmt doesn't need to be free'd whereas msg does. If you make msg const char *, the compiler will complain when you pass that to qemu_free(). I tend to think of the difference between 'const char *' and 'char *' as a string that I don't own vs. a string that I do own the reference to. It's not universally true but it tends to work nicely most of the time. >> + >> +void error_set(Error **errp, const char *fmt, ...) >> +{ >> + Error *err; >> + va_list ap; >> + >> + if (errp == NULL) { >> + return; >> + } >> + >> + err = qemu_mallocz(sizeof(*err)); >> + >> + va_start(ap, fmt); >> + err->obj = qobject_to_qdict(qobject_from_jsonv(fmt,&ap)); >> > vsprintf() and friends pass va_list by value, they don't use a > pointer. Perhaps you want to follow that idiom? > This va_list is passed to a recursive decent parser. The nature of va_list is such that if you pass by value, you cannot access it within a function after you've passed it to another function. Passing by reference seems to fix this (at least with GCC). I'm not 100% confident this is a strictly standards compliant solution but it's been working so far. Note that this isn't introduecd by this series. >> +bool error_is_type(Error *err, const char *fmt) >> +{ >> + char *ptr; >> + char *end; >> + char classname[1024]; >> + >> + ptr = strstr(fmt, "'class': '"); >> + assert(ptr != NULL); >> + ptr += strlen("'class': '"); >> + >> + end = strchr(ptr, '\''); >> + assert(end != NULL); >> + >> + memcpy(classname, ptr, (end - ptr)); >> + classname[(end - ptr)] = 0; >> + >> + return strcmp(classname, error_get_field(err, "class")) == 0; >> > I'd get rid of the buffer/memcpy and use strncmp in-place instead: > > const char *error_class = error_get_field(err, "class"); > if (strlen(error_class) != end - ptr) { > return false; > } > return strncmp(ptr, error_class, end - ptr) == 0; > Yeah, that's definitely better. Regards, Anthony Liguori > Stefan > >