[Libguestfs] Fwd: Review of libguestfs ruby bindings

[Richard W.M. Jones]

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