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

[Jim Meyering <jim meyering net>]

Chris Lalancette <clalance 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.