[Ovirt-devel] [patch] Even Better Exceptions For ruby-libvirt bindings

[Mohammed Morsi]

David Lutterkort wrote:
> On Mon, 2008-03-31 at 16:58 -0400, Mohammed Morsi wrote:
>   
>> Oy, let me try this one more time (last time I promise ;-) ).
>>     
>
> Seems like third time is indeed the charm :)
>
>   
>> diff -r de489d66999d ext/libvirt/_libvirt.c
>> --- a/ext/libvirt/_libvirt.c    Mon Mar 31 10:00:40 2008 -0700
>> +++ b/ext/libvirt/_libvirt.c    Mon Mar 31 16:55:03 2008 -0400
>>
>> @@ -111,6 +119,16 @@ static virConnectPtr conn(VALUE s) {
>>      return conn;
>>  }
>>  
>> +/* Errors */
>> +static VALUE create_error(VALUE error, char* method, char* msg, 
>> +                                virConnectPtr conn){
>> +    extern VALUE ruby_errinfo;
>> +    ruby_errinfo = rb_exc_new2(error, msg);
>> +    rb_iv_set(ruby_errinfo, "@method_name", rb_str_new2(method));
>> +    rb_iv_set(ruby_errinfo, "@vir_connect_ptr", connect_new(conn));
>> +    return ruby_errinfo;
>> +};
>> +
>>     
>
> I don't like the idea of storing the connection in the exception. If
> there is virError information that seems useful, it should be retrieved
> when the exception is created, and put into the exception object.
>
>   
Alright simple enough, now that I know how any information can be stored 
in the exception. I'm not fully sure what the cases are in which 
virError is useful and should be stored. But these can be worked out as 
I go along.
>>  /* Error handling */
>> -#define _E(cond, conn, fn) \
>> -    do { if (cond) vir_error(conn, fn); } while(0)
>> -
>> -NORETURN(static void vir_error(virConnectPtr conn, const char *fn));
>> -
>> -static void vir_error(virConnectPtr conn, const char *fn) {
>> -    rb_raise(rb_eSystemCallError, "libvir call %s failed", fn);
>> +#define _E(cond, excep) \
>> +    do { if (cond) vir_error(excep); } while(0)
>> + 
>> +NORETURN(static void vir_error(VALUE exception));
>> + 
>> +static void vir_error(VALUE exception) {
>> +    printf("libvir call %s failed\n", STR2CSTR(rb_iv_get(exception,
>> "@method_name")));
>> +    rb_exc_raise(exception);
>>  }
>>     
>
> I assume the printf is a remain from debugging - that needs to be
> removed.
>   
I put in the printf because I swapped rb_raise to rb_exc_raise. I had to 
do this because of how the exception object is constructed. rb_raise 
takes a message with the exception and simply feeds it into printf, 
something which rb_exc_raise does not, and I was assuming this was the 
expected/required behaviour so I put it in. Simple enough to change.
> The _E macro has two purposes: (1) remove the visual clutter from error
> checking and (2) avoid creating things at the point where errors are
> checked.
>
> The way it is now, e.g. in 
>
>   
>> @@ -396,7 +415,7 @@ VALUE libvirt_conn_version(VALUE s) {
>>      virConnectPtr conn = connect_get(s);
>>  
>>      r = virConnectGetVersion(conn, &v);
>> -    _E(r < 0, conn, "virConnectGetVersion");
>> +    _E(r < 0, create_error(e_RetrieveError, "virConnectGetVersion", "", conn));
>>  
>>      return ULONG2NUM(v);
>>  }
>>     
>
> an exception is allocated every time there is an error check. Can you
> change it so that create_error is only called when there actually is a
> need to throw an exception ? _E should be changed so that the above use
> looks something like
>
>         _E(r < 0, e_RetrieveError, "virConnectGetVersion", conn);
>   
I was actually thinking about the _E and the continuous exception 
creation as well and I don't think its an issue. Its been a while so 
feel free to correct me if I'm wrong, but I believe the #define 
preprocessor macro will evaluate before compilation, replacing all the 
calls to _E(condition, exception) with if(condition) 
vir_error(create_error(....)). Thus when the code is actually compiled, 
the error objects do no get created unless the condition is true.
> It also looks like the third argument to create_error is always the
> empty string, so it could be dropped, at least from the invocation of
> _E.
>   
The string is actually required for exception instantiation. I can 
change it so that when the code creates the next exception, it uses the 
method_name string instead. I just thought it was useful to have an 
additional, error-specific, message other than "libvir call %s failed".

> Also, the only reason to pass the connection here would be to get more
> error information from libvirt. If that's not needed, that argument
> should be removed, too. I probably should have done that from the very
> beginning.
>
>   
Well thats the thing, is it the case that in some cases it is needed and 
some it isn't? Once again, I'm a little unclear on how virError mention 
previously fits into the equation and was under the impression that the 
info you needed to handle the exception properly was stored in the 
vir_connect_ptr
>> +    // create 'method_name' and 'vir_connect_ptr' attributes on
>> e_Error class
>> +    rb_define_attr(e_Error, "method_name", 1, 1);
>> +    rb_define_attr(e_Error, "vir_connect_ptr", 1, 1);
>>     
>
> This is a very small nit, but I'd prefer a less OO centric name for the
> attribute holding the name of the libvirt function than
> 'method_name' ... maybe 'libvirt_function' or 'libvirt_function_name'
>   
Easy as pie :-)
> David
>
>   

For the most part everything mentioned will be simple to do, but I will 
need some more info / specifics. I'll ping you about it sometime tomorrow.

   -Mo