Re: [Qemu-devel] [PATCH 05/13] error: define struct Error in only one place

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> Il 18/10/2013 03:11, Wenchao Xia ha scritto:
>> Signed-off-by: Wenchao Xia <address@hidden>
>> ---
>>  include/qapi/error.h |    5 ++++-
>>  qobject/qerror.c     |    7 -------
>>  util/error.c         |    6 ------
>>  3 files changed, 4 insertions(+), 14 deletions(-)
>> 
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 7d4c696..8688aaf 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -20,7 +20,10 @@
>>   * A class representing internal errors within QEMU.  An error has a 
>> ErrorClass
>>   * code and a human message.
>>   */
>> -typedef struct Error Error;
>> +typedef struct Error {
>> +    char *msg;
>> +    ErrorClass err_class;
>> +} Error;
>
> Please add a comment that it should be treated as an opaque type.

Or keep it opaque here, and complete the type in an internal header.
But see below.

>
> Paolo
>
>>  
>>  /**
>>   * Set an indirect pointer to an error given a ErrorClass value and a
>> diff --git a/qobject/qerror.c b/qobject/qerror.c
>> index 3aee1cf..5b487f3 100644
>> --- a/qobject/qerror.c
>> +++ b/qobject/qerror.c
>> @@ -97,13 +97,6 @@ void qerror_report(ErrorClass eclass, const char *fmt, 
>> ...)
>>      }
>>  }
>>  
>> -/* Evil... */
>> -struct Error
>> -{
>> -    char *msg;
>> -    ErrorClass err_class;
>> -};
>> -
>>  void qerror_report_err(Error *err)
>>  {
>>      QError *qerr;

        qerr = qerror_new();
        loc_save(&qerr->loc);
        qerr->err_msg = g_strdup(err->msg);
        qerr->err_class = err->err_class;

        if (monitor_cur_is_qmp()) {
            monitor_set_error(cur_mon, qerr);
        } else {
            qerror_print(qerr);
            QDECREF(qerr);
        }
    }

This is the only use of the "evil" duplicate.  I suspect it could be
cleaned up like this:

        qerr->err_msg = g_strdup(error_get_pretty(err));
        qerr->err_class = error_get_class(err);

If that's true, the duplicate goes away, and we can keep the type
opaque.

>> diff --git a/util/error.c b/util/error.c
>> index ec0faa6..da0d221 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -17,12 +17,6 @@
>>  #include "qapi-types.h"
>>  #include "qapi/qmp/qerror.h"
>>  
>> -struct Error
>> -{
>> -    char *msg;
>> -    ErrorClass err_class;
>> -};
>> -
>>  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>>  {
>>      Error *err;
>>