Chris helpfully reviewed the libguestfs ruby bindings. His findings are below. Rich. Date: Wed, 5 Jan 2011 16:36:42 -0500 From: Chris Lalancette Subject: Review of libguestfs ruby bindings Hey Rich, What follows is a quick review of the libguestfs ruby bindings. I hope you find it helpful. Overall directory structure - looks reasonable enough. One thing that you *could* do is remove lib/guestfs.rb completely, rename ext/guestfs/_guestfs.c to ext/guestfs/guestfs.c, and change your init function to Init_guestfs. It just removes the (unnecessary) redirection through the ruby file and will achieve the same results. Minor, though. Rakefile - looks reasonable enough, has most of the tasks I would care about. One additional task that might be worthwhile is an 'ri' task for generating interactive help: Rake::RDocTask.new(:ri) do |rd| rd.main = "README.rdoc" rd.rdoc_dir = "doc/ri" rd.options << "--ri-system" rd.rdoc_files.include(RDOC_FILES) end Full disclosure, though; I haven't yet figured out how to include that interactive help in an RPM. extconf.rb file - looks reasonable enough. _guestfs.c - lots of code here (auto-generated), so I won't do a line-by-line review. I'll point out a few things though. 1) guestfs_safe_malloc() is used frequently. This is fine, though it is *slightly* better to use ALLOC_N and friends from ruby.h. Those functions try to allocate memory, and if they fail, run the gc to free up memory and try again. That is essentially never going to happen on 64-bit, but with a fragmented enough 32-bit address space it could save NULL being returned. Very minor though, up to you if you want to change it. 2) INT2NUM and NUM2INT are used frequently. These are good, and should always be used in favor of the INT2FIX and FIX2INT counterparts. 3) Missing RDoc markups. While you have an rdoc task in the Rakefile, I don't think it will generate very much because you are missing the markups for it. You can look at ruby-libvirt for a lot of examples of this, but a simple one is: /* * call-seq: g.launch -> nil * * Call guestfs_launch to launch the appliance */ 4) Probably too late to fix this, but the way that "optional" arguments are done is not idiomatic to ruby. For instance, ruby_guestfs_add_drive_opts() takes a (possibly empty) hash that contains the arguments you care about. If they are all nil, then no arguments are passed. More idiomatic would be to have ruby_guestfs_add_drive_opts() take a variable number of parameters and then only require the user to pass in the ones that they are interested in. If you care to look, an example in the ruby-libvirt code is ext/libvirt/domain.c:libvirt_dom_memory_peek() 5) The way that hash lookups are done are a little complicated. For instance, ruby_guestfs_add_drive_opts() uses: VALUE v; v = rb_hash_lookup (optargsv, ID2SYM (rb_intern ("readonly"))); to get at the hash symbol. In ruby-libvirt I tend to do: v = rb_hash_aref(optsargv, rb_str_new2("readonly")); That being said, your way may actually be faster since you aren't allocating a new object. Minor in any case, they both work. 6) In ruby_guestfs_download() and elsewhere, there is code like: Check_Type (remotefilenamev, T_STRING); const char *remotefilename = StringValueCStr (remotefilenamev); if (!remotefilename) rb_raise (rb_eTypeError, "expected string for parameter %s of %s", "remotefilename", "download"); First, the Check_Type isn't strictly necessary; StringValueCStr will check if this is a T_STRING, and if not, call the ".to_str" method to convert whatever it is to a string. This makes it slightly more flexible, if you want to allow that. The more important bit though is that, as far as I can tell, StringValueCStr will never return a NULL pointer. As mentioned StringValueCStr will first convert to a string, and after that if the RSTRING(s)->ptr is still NULL, will set the pointer to "". That being said, I think you can remove the check entirely; either you will get back a valid string (even if it is empty), or the interpreter will raise an exception (in which case the rest of your function won't be called anyway). 7) In ruby_guestfs_test0() and other places, there is code like: Check_Type (strlistv, T_ARRAY); { size_t i, len; len = RARRAY_LEN (strlistv); strlist = guestfs_safe_malloc (g, sizeof (char *) * (len+1)); for (i = 0; i < len; ++i) { VALUE v = rb_ary_entry (strlistv, i); strlist[i] = StringValueCStr (v); } strlist[len] = NULL; } Unfortunately this can cause a memory leak. I have an upcoming blog post about this, but the short of it is that anything allocated with malloc() and friends do not take part in ruby garbage collecting. So if you allocate strlist, and then rb_ary_entry() or StringValueCStr() throws an exception, the ruby VM will directly longjmp out of the faulting code back into the interpreter. You've now leaked strlist. The idiom to deal with this is a bit clunky; it essentially involves invoking a callback that is guaranteed to never longjmp out, then checking for errors and longjmp'ing yourself. A very simple example is: struct rb_ary_entry_args { VALUE arr; int offset; }; static VALUE rb_ary_entry_wrap(VALUE in) { struct rb_ary_entry_args *e = (struct rb_ary_entry_args *)in; return rb_ary_entry(e->arr, e->offset); } Check_Type (strlistv, T_ARRAY); { size_t i, len; struct rb_ary_entry_args args; int exception; len = RARRAY_LEN (strlistv); strlist = guestfs_safe_malloc (g, sizeof (char *) * (len+1)); for (i = 0; i < len; ++i) { args.arr = strlist; args.offset = i; VALUE v = rb_protect(rb_ary_entry_wrap (VALUE)&args, &exception); if (exception) { free(strlist); rb_jump_tag(exception); } strlist[i] = StringValueCStr (v); } strlist[len] = NULL; } I have many examples of this idiom in the ruby-libvirt code; ext/libvirt/domain.c:libvirt_dom_memory_peek() is a good example. That's all I saw from a quick look-through. Let me know if you have questions about any of the above. -- Chris Lalancette -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones New in Fedora 11: Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 70 libraries supprt'd http://fedoraproject.org/wiki/MinGW http://www.annexia.org/fedora_mingw