[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]