5:07 a.m.
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.
Signed-off-by: Chris Lalancette <clalance(a)redhat.com>
8:15 a.m.
Chris Lalancette <clalance(a)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);
...
9:22 a.m.
Jim Meyering wrote:
> 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)
Yeah, you are right. I looked through the ruby source code, actually, and I
ended up using StringValueCStr (which is used elsewhere in the ruby-libvirt
bindings). It's basically the same as StringValuePtr, but does an additional
check to make sure the string is not of 0 length and that there aren't
additional embedded \0 in the string.
I also updated the patch with the const pointers as you suggested. Attached.
Thanks for the review!
Chris Lalancette
9:52 a.m.
Chris Lalancette <clalance(a)redhat.com> wrote:
Jim Meyering wrote:
>> 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)
Yeah, you are right. I looked through the ruby source code, actually, and I
ended up using StringValueCStr (which is used elsewhere in the ruby-libvirt
That does sound better, at least when (as here) you know there
should be no empty string and no embedded NUL byte.
bindings). It's basically the same as StringValuePtr, but does
an additional
check to make sure the string is not of 0 length and that there aren't
additional embedded \0 in the string.
I also updated the patch with the const pointers as you suggested. Attached.
Thanks for the review!
Looks fine.
ACK.
5:07 p.m.
On Fri, 2008-08-08 at 16:22 +0200, Chris Lalancette wrote:
Jim Meyering wrote:
>> 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)
Yeah, you are right. I looked through the ruby source code, actually, and I
ended up using StringValueCStr (which is used elsewhere in the ruby-libvirt
bindings). It's basically the same as StringValuePtr, but does an additional
check to make sure the string is not of 0 length and that there aren't
additional embedded \0 in the string.
I also updated the patch with the const pointers as you suggested. Attached.
Thanks for the review!
ACK .. committed.
I'll try and roll a new release tomorrow.
David
5946
days inactive
5950
days old
4 comments
3 participants
participants (3)
-
Chris Lalancette -
David Lutterkort -
Jim Meyering