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

[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

-- 
Michal Novotny <minovotn at redhat.com>, RHCE
Virtualization Team (xen userspace), Red Hat