[libvirt] [libvirt-php PATCH 03/29] add missing arginfo

[Michal Privoznik]

On 15.04.2016 08:05, Remi Collet wrote:
> Le 14/04/2016 18:30, Michal Privoznik a écrit :
>> On 13.04.2016 18:13, Neal Gompa wrote:
>>> From: Remi Collet <fedora at famillecollet.com>
>>>
>>> ---
>>>  src/libvirt-php.c | 715 +++++++++++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 553 insertions(+), 162 deletions(-)
>>>
>>> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
>>> index 1af6077..3f06edc 100644
>>> --- a/src/libvirt-php.c
>>> +++ b/src/libvirt-php.c
>>
>>
>>> @@ -5984,7 +6371,7 @@ PHP_FUNCTION(libvirt_domain_memory_peek)
>>>      zval *zdomain;
>>>      int retval;
>>>      long flags=0;
>>> -    long long start;
>>> +    long start;
>>
>> This is spurious for two reasons:
>> 1) this patch aims at something different. So this change does not
>> belong here.
>> 2) The @start variable is passed to virDomainMemoryPeek(). It expects
>> unsigned long long. So this change does not belong here.
> 
> I confirm this patch is correct and needed.
> 
>     GET_DOMAIN_FROM_ARGS("rlll",&zdomain,&start,&size,&flags);

So this is broken too. But there's more similar bugs to this in our code.

> 
> zend_parse_parameters expect a (long *) for "l" option
> 
> Casting a (long long *) to a (long *) is terrible, mash the stack, can
> raise segfault, and very bad thing on big endian

How can it mash the stack? I'd expect sizeof(long long *) == sizeof
(long *) == sizeof(void *). It's a pointer after all. True about
endiandness though. But then again - bug is in GET_DOMAIN_FROM_ARGS()
arg string.

Then again, this commit focuses on adding missing arginfo. This should
not had been here rather than a separate patch.

> 
>     retval=virDomainMemoryPeek(domain->domain,start,size,buff,flags);
> 
> Casting a (long) to a (long long) is perfectly OK.
> 
> So current version is broken
> 
> -    zend_ulong64 start;
> +    zend_long start;
> 
> And if you want to manage the negative value, just add a test
> 
> 	if (start<0) RETURN_FALSE;
> 

I don't think we need to come up with an extension to libvirt API. The
@start argument tells an address from which the API should fetch data.
Unsigned integer value is just fine for that.

Michal