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

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