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

[Anthony Liguori]

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