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

[Anthony Liguori <anthony@codemonkey.ws>]

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