[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 03/22] qapi: add Error object

From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 03/22] qapi: add Error object
Date: Mon, 07 Mar 2011 07:36:00 -0600
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv: Gecko/20101027 Lightning/1.0b1 Thunderbird/3.0.10

On 03/07/2011 05:38 AM, Stefan Hajnoczi wrote:
On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori<address@hidden>  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.


Anthony Liguori


reply via email to

[Prev in Thread] Current Thread [Next in Thread]