[libvirt] [PATCH]: ruby-libvirt migration fixes

[Jim Meyering]

Chris Lalancette <clalance at redhat.com> wrote:
> Attached is a relatively simple patch to the ruby-libvirt bindings with some
> bugfixes for the migrate call.  The first problem was that there was no way to
> pass a "nil" through to the underlying virDomainMigrate() call.  This is
> important because generally the "dname" and "uri" parameters end up being NULL.
>  This patch also makes all the parameters beyond the first 2 optional.  I've
> tested this out by live migrating a KVM guest between two hosts, and this now
> does what I expect.

Hi Chris,

I'm no ruby integration guru, but took a look anyhow.
Looks fine modulo a couple nits.

> diff -r c6a3e36cdf54 ext/libvirt/_libvirt.c
> --- a/ext/libvirt/_libvirt.c	Thu Jul 17 15:24:26 2008 -0700
> +++ b/ext/libvirt/_libvirt.c	Fri Aug 08 06:04:56 2008 -0400
> @@ -637,16 +637,51 @@ VALUE libvirt_conn_num_of_defined_storag
>  }
>  #endif
>
> +static char *get_string_or_nil(VALUE arg)
> +{
> +    if (TYPE(arg) == T_NIL)
> +        return NULL;
> +    else if (TYPE(arg) == T_STRING)
> +        return STR2CSTR(arg);

STR2CSTR is marked as obsolete in ruby.h, where it says
to use StringValuePtr instead:

    /* obsolete API - use StringValuePtr() */
    #define STR2CSTR(x) rb_str2cstr((VALUE)(x),0)

> +    else
> +        rb_raise(rb_eTypeError, "wrong argument type (expected String or nil)");    return NULL;
> +}
> +
>  /*
>   * Class Libvirt::Domain
>   */
> -VALUE libvirt_dom_migrate(VALUE s, VALUE dconn, VALUE flags,
> -                           VALUE dname, VALUE uri, VALUE bandwidth) {
> +VALUE libvirt_dom_migrate(int argc, VALUE *argv, VALUE s) {
>      virDomainPtr ddom = NULL;
> +    VALUE dconn;
> +    unsigned long flags;
> +    char *dname;
> +    char *uri;

Both pointers can be "const".  Marking them as const also
helps to emphasize that they must not be freed.

> +    unsigned long bandwidth;
>
> -    ddom = virDomainMigrate(domain_get(s), conn(dconn), NUM2UINT(flags),
> -                            StringValueCStr(dname), StringValueCStr(uri),
> -                            NUM2UINT(bandwidth));
> +    if (argc < 2 || argc > 5) {
> +        rb_raise(rb_eArgError, "Must provide between 2 and 5 arguments");
> +    }
> +
> +    dconn = argv[0];
> +    flags = NUM2UINT(argv[1]);
> +    dname = NULL;
> +    uri = NULL;
> +    bandwidth = 0;
> +
> +    switch(argc) {
> +    case 5:
> +        Check_Type(argv[4], T_FIXNUM);
> +        bandwidth = NUM2UINT(argv[4]);
> +        /* fallthrough */
> +    case 4:
> +        uri = get_string_or_nil(argv[3]);
> +        /* fallthrough */
> +    case 3:
> +        dname = get_string_or_nil(argv[2]);
> +    }
> +
> +    ddom = virDomainMigrate(domain_get(s), conn(dconn), flags,
> +                            dname, uri, bandwidth);
...