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

David Lutterkort dlutter at redhat.com
Mon Mar 31 21:30:05 UTC 2008


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.

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

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);

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.

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.

> +    // 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'

David




More information about the ovirt-devel mailing list