[libvirt] [libvirt-php] Add volume clone support

[Lyre]

于 2011年03月28日 17:19, Michal Novotny 写道:
> Hi Lyre,
> I'm having some (inline) comments to consider...
>
>> -	struct _virDomainMemoryStat stats[VIR_DOMAIN_MEMORY_STAT_NR];
>> +
>> +	struct _virDomainMemoryStat stats[VIR_DOMAIN_MEMORY_STAT_NR] =
>> +	{
>> +		{VIR_DOMAIN_MEMORY_STAT_SWAP_IN, 0},
>> +		{VIR_DOMAIN_MEMORY_STAT_SWAP_OUT, 0},
>> +		{VIR_DOMAIN_MEMORY_STAT_MAJOR_FAULT, 0},
>> +		{VIR_DOMAIN_MEMORY_STAT_MINOR_FAULT, 0},
>> +		{VIR_DOMAIN_MEMORY_STAT_UNUSED, 0},
>> +		{VIR_DOMAIN_MEMORY_STAT_AVAILABLE, 0},
>> +	};
>
> Why did you change this in patch that's about adding
> libvirt_storagevolume_create_xml_from() API ? Please resubmit this again
> in a separate patch stating reasons why to change this. It's OK with me
> to make such a change if you put some information why are you changing
> this either to comment or a commit message but it's rather confusing
> when you don't mention reason why to change this anywhere so please
> resubmit as a new patch.
>
>
>>
>>   	GET_DOMAIN_FROM_ARGS("r|l",&zdomain,&flags);
>>
>> @@ -2804,6 +2814,51 @@ PHP_FUNCTION(libvirt_storagevolume_create_xml)
>>   }
>>
>>   /*
>> +	Function name:	libvirt_storagevolume_create_xml_from
>> +	Since version:	0.4.1(-2)
>> +	Description:	Function is used to clone the new storage volume into pool from the orignial volume
>> +	Arguments:		@pool [resource]: libvirt storagepool resource
>> +					@xml [string]: XML string to create the storage volume in the storage pool
>> +					@original_volume [resource]: libvirt storagevolume resource
>> +	Returns:		libvirt storagevolume resource
>> +*/
>> +PHP_FUNCTION(libvirt_storagevolume_create_xml_from)
>> +{
>> +	php_libvirt_volume *res_volume=NULL;
>> +	php_libvirt_storagepool *pool=NULL;
>> +	zval *zpool;
>> +
>> +	php_libvirt_volume *pl_volume=NULL;
>> +	zval *zvolume;
>> +
>> +	virStorageVolPtr volume=NULL;
>> +	char *xml;
>> +	int xml_len;
>> +
>> +	if (zend_parse_parameters (ZEND_NUM_ARGS() TSRMLS_CC, "rsr",&zpool,&xml,&xml_len,&zvolume) == FAILURE)
>> +	{
>> +		RETURN_FALSE;
>> +	}
>> +
>> +	ZEND_FETCH_RESOURCE (pool, php_libvirt_storagepool*,&zpool, -1, PHP_LIBVIRT_STORAGEPOOL_RES_NAME, le_libvirt_storagepool);
>> +	if ((pool==NULL)||(pool->pool==NULL))RETURN_FALSE;
>> +	ZEND_FETCH_RESOURCE (pl_volume, php_libvirt_volume*,&zvolume, -1, PHP_LIBVIRT_VOLUME_RES_NAME, le_libvirt_volume);
>> +	if ((pl_volume==NULL)||(pl_volume->volume==NULL))RETURN_FALSE;
>> +
>> +	volume=virStorageVolCreateXMLFrom(pool->pool,xml, pl_volume->volume, 0);
>> +	if (volume==NULL)
>> +	{
>> +		set_error ("Cannot create new volume" TSRMLS_CC);
>
> Why are you setting up the error using set_error() there? Please see
> virStorageVolCreateXMLFrom() documentation at
> http://libvirt.org/html/libvirt-libvirt.html#virStorageVolCreateXMLFrom
> . It should be setting NULL on error and the error should be caught by
> the catch_error() function which already calls the set_error() with
> appropriate arguments, i.e. it's already setting up the libvirt error
> string into the last_error string. Please avoid this one.
>
>
>> +		RETURN_FALSE;
>> +	}
>> +
>> +	res_volume= emalloc(sizeof(php_libvirt_volume));
>> +	res_volume->volume = volume;
>> +
>> +	ZEND_REGISTER_RESOURCE(return_value, res_volume, le_libvirt_volume);
>> +}
>> +
>> +/*
>>   	Function name:	libvirt_storagepool_get_uuid_string
>>   	Since version:	0.4.1(-1)
>>   	Description:	Function is used to get storage pool by UUID string
>> @@ -3058,7 +3113,7 @@ PHP_FUNCTION(libvirt_storagepool_refresh)
>>   	zval *zpool;
>>   	unsigned long flags = 0;
>>
>> -	GET_STORAGEPOOL_FROM_ARGS ("rl",&zpool,&flags);
>> +	GET_STORAGEPOOL_FROM_ARGS ("r|l",&zpool,&flags);
>
> You should mention this change in the commit message or code comment as
> well.
>
> Please resubmit v2 for the new API only and another patch for the fixes
> (virDomainMemoryStat bits and libvirt_storagepool_refresh bits).
>
> You're doing great however there are some things that should be changed
> so please don't take this as a bad thing. It's just a kind of adjustment
> how we should write the codes/patches.
>
> Thanks a lot!
> Michal
>
Hi Michal:

Sorry about that, I didn't noticed the error handling mechanism in 
libvirt before.

I've been overloaded these days. And I'll resent those patches.

t