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<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.

Regards,

Anthony Liguori

> Stefan
>
>