On 07.05.2015 16:21, Vasiliy Tolstov wrote:
> * add constants from libvirt to snapshots api
> * add flags to snapshot functions
>
> Signed-off-by: Vasiliy Tolstov <v.tolstov@xxxxxxxxx>
> ---
> src/libvirt-php.c | 87 ++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 61 insertions(+), 26 deletions(-)
>
There should be [libvirt-php] in the subject. You can achieve that
(permanently) via:
libvirt-php.git $ git config format.subjectprefix libvirt-php][PATCH
> diff --git a/src/libvirt-php.c b/src/libvirt-php.c
> index 0adc4be..e9b9657 100644
> --- a/src/libvirt-php.c
> +++ b/src/libvirt-php.c
> @@ -1228,34 +1228,58 @@ PHP_MINIT_FUNCTION(libvirt)
> REGISTER_LONG_CONSTANT("VIR_DOMAIN_CRASHED", 6, CONST_CS | CONST_PERSISTENT);
>
> /* Volume constants */
> - REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_ALLOCATE", 1, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_DELTA", 2, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_SHRINK", 4, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_ALLOCATE", 1, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_DELTA", 2, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_STORAGE_VOL_RESIZE_SHRINK", 4, CONST_CS | CONST_PERSISTENT);
This change looks questionable to me. Moreover, it should be in a
separate patch anyway.
>
> /* Domain vCPU flags */
> REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_CONFIG", VIR_DOMAIN_VCPU_CONFIG, CONST_CS | CONST_PERSISTENT);
> REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_CURRENT", VIR_DOMAIN_VCPU_CURRENT, CONST_CS | CONST_PERSISTENT);
> REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_LIVE", VIR_DOMAIN_VCPU_LIVE, CONST_CS | CONST_PERSISTENT);
> REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_MAXIMUM", VIR_DOMAIN_VCPU_MAXIMUM, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_GUEST", VIR_DOMAIN_VCPU_GUEST, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_DOMAIN_VCPU_GUEST", VIR_DOMAIN_VCPU_GUEST, CONST_CS | CONST_PERSISTENT);
So while previously the constants were aligned, now they are not. NACK
to this hunk.
>
> #if LIBVIR_VERSION_NUMBER>=8000
> /* Domain snapshot constants */
> REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_DELETE_CHILDREN", VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_DELETE_METADATA_ONLY", VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_DELETE_CHILDREN_ONLY", VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN_ONLY, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_REDEFINE", VIR_DOMAIN_SNAPSHOT_CREATE_REDEFINE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_CURRENT", VIR_DOMAIN_SNAPSHOT_CREATE_CURRENT, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_NO_METADATA", VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_HALT", VIR_DOMAIN_SNAPSHOT_CREATE_HALT, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_DISK_ONLY", VIR_DOMAIN_SNAPSHOT_CREATE_DISK_ONLY, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_REUSE_EXT", VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_QUIESCE", VIR_DOMAIN_SNAPSHOT_CREATE_QUIESCE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_ATOMIC", VIR_DOMAIN_SNAPSHOT_CREATE_ATOMIC, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_CREATE_LIVE", VIR_DOMAIN_SNAPSHOT_CREATE_LIVE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_DESCENDANTS", VIR_DOMAIN_SNAPSHOT_LIST_DESCENDANTS, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_ROOTS", VIR_DOMAIN_SNAPSHOT_LIST_ROOTS, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_METADATA", VIR_DOMAIN_SNAPSHOT_LIST_METADATA, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_LEAVES", VIR_DOMAIN_SNAPSHOT_LIST_LEAVES, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_NO_LEAVES", VIR_DOMAIN_SNAPSHOT_LIST_NO_LEAVES, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_NO_METADATA", VIR_DOMAIN_SNAPSHOT_LIST_NO_METADATA, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_INACTIVE", VIR_DOMAIN_SNAPSHOT_LIST_INACTIVE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_ACTIVE", VIR_DOMAIN_SNAPSHOT_LIST_ACTIVE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_DISK_ONLY", VIR_DOMAIN_SNAPSHOT_LIST_DISK_ONLY, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_INTERNAL", VIR_DOMAIN_SNAPSHOT_LIST_INTERNAL, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_LIST_EXTERNAL", VIR_DOMAIN_SNAPSHOT_LIST_EXTERNAL, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_REVERT_RUNNING", VIR_DOMAIN_SNAPSHOT_REVERT_RUNNING, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_REVERT_PAUSED", VIR_DOMAIN_SNAPSHOT_REVERT_PAUSED, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_SNAPSHOT_REVERT_FORCE", VIR_DOMAIN_SNAPSHOT_REVERT_FORCE, CONST_CS | CONST_PERSISTENT);
> #endif
This could be better formatted, but okay.
>
> /* Memory constants */
> REGISTER_LONG_CONSTANT("VIR_MEMORY_VIRTUAL", 1, CONST_CS | CONST_PERSISTENT);
>
> /* Version checking constants */
> - REGISTER_LONG_CONSTANT("VIR_VERSION_BINDING", VIR_VERSION_BINDING, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_VERSION_LIBVIRT", VIR_VERSION_LIBVIRT, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_VERSION_BINDING", VIR_VERSION_BINDING, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_VERSION_LIBVIRT", VIR_VERSION_LIBVIRT, CONST_CS | CONST_PERSISTENT);
No, please don't use TABs. Everywhere in libvirt and its subprojects we
tend to use spaces. NACK to this hunk, sorry.
>
> /* Network constants */
> REGISTER_LONG_CONSTANT("VIR_NETWORKS_ACTIVE", VIR_NETWORKS_ACTIVE, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_NETWORKS_INACTIVE", VIR_NETWORKS_INACTIVE, CONST_CS | CONST_PERSISTENT);
> - REGISTER_LONG_CONSTANT("VIR_NETWORKS_ALL", VIR_NETWORKS_ACTIVE |
> - VIR_NETWORKS_INACTIVE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_NETWORKS_INACTIVE", VIR_NETWORKS_INACTIVE, CONST_CS | CONST_PERSISTENT);
> + REGISTER_LONG_CONSTANT("VIR_NETWORKS_ALL", VIR_NETWORKS_ACTIVE | VIR_NETWORKS_INACTIVE, CONST_CS | CONST_PERSISTENT);
>
> /* Credential constants */
> REGISTER_LONG_CONSTANT("VIR_CRED_USERNAME", 1, CONST_CS | CONST_PERSISTENT);
So this patch mixes two changes into one. How about splitting them into
two? In the first patch you can add new constants, in the second you can
update the functions. Or vice versa. Whatever you prefer.
Michal
> @@ -6394,17 +6418,19 @@ PHP_FUNCTION(libvirt_domain_get_job_info)
> Since version: 0.4.1(-2)
> Description: Function is used to get the information whether domain has the current snapshot
> Arguments: @res [resource]: libvirt domain resource
> + @flags [int]
> Returns: TRUE is domain has the current snapshot, otherwise FALSE (you may need to check for error using libvirt_get_last_error())
> */
> PHP_FUNCTION(libvirt_domain_has_current_snapshot)
> {
> php_libvirt_domain *domain=NULL;
> zval *zdomain;
> + long flags = 0;
> int retval;
>
> - GET_DOMAIN_FROM_ARGS("r",&zdomain);
> + GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &flags);
>
> - retval=virDomainHasCurrentSnapshot(domain->domain, 0);
> + retval=virDomainHasCurrentSnapshot(domain->domain, flags);
> if (retval <= 0) RETURN_FALSE;
> RETURN_TRUE;
> }
> @@ -6415,6 +6441,7 @@ PHP_FUNCTION(libvirt_domain_has_current_snapshot)
> Description: This functions is used to lookup for the snapshot by it's name
> Arguments: @res [resource]: libvirt domain resource
> @name [string]: name of the snapshot to get the resource
> + @flags [int]
> Returns: domain snapshot resource
> */
> PHP_FUNCTION(libvirt_domain_snapshot_lookup_by_name)
> @@ -6423,13 +6450,14 @@ PHP_FUNCTION(libvirt_domain_snapshot_lookup_by_name)
> zval *zdomain;
> int name_len;
> char *name=NULL;
> + long flags = 0;
> php_libvirt_snapshot *res_snapshot;
> virDomainSnapshotPtr snapshot = NULL;
>
> - GET_DOMAIN_FROM_ARGS("rs",&zdomain,&name,&name_len);
> + GET_DOMAIN_FROM_ARGS("rs|l",&zdomain,&name,&name_len, &flags);
>
> if ( (name == NULL) || (name_len<1)) RETURN_FALSE;
> - snapshot=virDomainSnapshotLookupByName(domain->domain, name, 0);
> + snapshot=virDomainSnapshotLookupByName(domain->domain, name, flags);
> if (snapshot==NULL) RETURN_FALSE;
>
> res_snapshot = (php_libvirt_snapshot *)emalloc(sizeof(php_libvirt_snapshot));
> @@ -6446,6 +6474,7 @@ PHP_FUNCTION(libvirt_domain_snapshot_lookup_by_name)
> Since version: 0.4.1(-2)
> Description: This function creates the domain snapshot for the domain identified by it's resource
> Arguments: @res [resource]: libvirt domain resource
> + @flags [int]
> Returns: domain snapshot resource
> */
> PHP_FUNCTION(libvirt_domain_snapshot_create)
> @@ -6453,11 +6482,12 @@ PHP_FUNCTION(libvirt_domain_snapshot_create)
> php_libvirt_domain *domain=NULL;
> php_libvirt_snapshot *res_snapshot;
> zval *zdomain;
> + long flags = 0;
> virDomainSnapshotPtr snapshot = NULL;
>
> - GET_DOMAIN_FROM_ARGS("r",&zdomain);
> + GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &flags);
>
> - snapshot=virDomainSnapshotCreateXML(domain->domain, "<domainsnapshot/>", 0);
> + snapshot=virDomainSnapshotCreateXML(domain->domain, "<domainsnapshot/>", flags);
> DPRINTF("%s: virDomainSnapshotCreateXML(%p, <xml>) returned %p\n", PHPFUNC, domain->domain, snapshot);
> if (snapshot == NULL) RETURN_FALSE;
>
> @@ -6475,6 +6505,7 @@ PHP_FUNCTION(libvirt_domain_snapshot_create)
> Since version: 0.4.1(-2)
> Description: Function is used to get the XML description of the snapshot identified by it's resource
> Arguments: @res [resource]: libvirt snapshot resource
> + @flags [int]
> Returns: XML description string for the snapshot
> */
> PHP_FUNCTION(libvirt_domain_snapshot_get_xml)
> @@ -6482,11 +6513,12 @@ PHP_FUNCTION(libvirt_domain_snapshot_get_xml)
> char *xml;
> char *xml_out;
> zval *zsnapshot;
> + long flags = 0;
> php_libvirt_snapshot *snapshot;
>
> - GET_SNAPSHOT_FROM_ARGS("r",&zsnapshot);
> + GET_SNAPSHOT_FROM_ARGS("r|l",&zsnapshot, &flags);
>
> - xml = virDomainSnapshotGetXMLDesc(snapshot->snapshot, 0);
> + xml = virDomainSnapshotGetXMLDesc(snapshot->snapshot, flags);
> if (xml==NULL) RETURN_FALSE;
>
> RECREATE_STRING_WITH_E(xml_out,xml);
> @@ -6499,19 +6531,21 @@ PHP_FUNCTION(libvirt_domain_snapshot_get_xml)
> Since version: 0.4.1(-2)
> Description: Function is used to revert the domain state to the state identified by the snapshot
> Arguments: @res [resource]: libvirt snapshot resource
> + @flags [int]
> Returns: TRUE on success, FALSE on error
> */
> PHP_FUNCTION(libvirt_domain_snapshot_revert)
> {
> zval *zsnapshot;
> php_libvirt_snapshot *snapshot;
> - int ret;
> + long flags = 0;
> + int retval;
>
> - GET_SNAPSHOT_FROM_ARGS("r",&zsnapshot);
> + GET_SNAPSHOT_FROM_ARGS("r|l",&zsnapshot, &flags);
>
> - ret = virDomainRevertToSnapshot(snapshot->snapshot, 0);
> - DPRINTF("%s: virDomainRevertToSnapshot(%p, 0) returned %d\n", PHPFUNC, snapshot->snapshot, ret);
> - if (ret == -1) RETURN_FALSE;
> + retval = virDomainRevertToSnapshot(snapshot->snapshot, flags);
> + DPRINTF("%s: virDomainRevertToSnapshot(%p, 0) returned %d\n", PHPFUNC, snapshot->snapshot, retval);
> + if (retval == -1) RETURN_FALSE;
> RETURN_TRUE;
> }
>
> @@ -6543,6 +6577,7 @@ PHP_FUNCTION(libvirt_domain_snapshot_delete)
> Since version: 0.4.1(-2)
> Description: Function is used to list domain snapshots for the domain specified by it's resource
> Arguments: @res [resource]: libvirt domain resource
> + @flags [int]
> Returns: libvirt domain snapshot names array
> */
> PHP_FUNCTION(libvirt_list_domain_snapshots)
> @@ -6551,17 +6586,18 @@ PHP_FUNCTION(libvirt_list_domain_snapshots)
> zval *zdomain;
> int count=-1;
> int expectedcount=-1;
> + long flags = 0;
> char **names;
> int i;
>
> - GET_DOMAIN_FROM_ARGS("r",&zdomain);
> + GET_DOMAIN_FROM_ARGS("r|l",&zdomain, &flags);
>
> - expectedcount=virDomainSnapshotNum(domain->domain, 0);
> + expectedcount=virDomainSnapshotNum(domain->domain, flags);
> DPRINTF("%s: virDomainSnapshotNum(%p, 0) returned %d\n", PHPFUNC, domain->domain, expectedcount);
>
> if (expectedcount != -1 ) {
> names=(char **)emalloc( expectedcount * sizeof(char *) );
> - count=virDomainSnapshotListNames(domain->domain, names, expectedcount, 0);
> + count=virDomainSnapshotListNames(domain->domain, names, expectedcount, flags);
> }
>
> if ((count != expectedcount) || (count<0)) {
> @@ -8863,4 +8899,3 @@ PHP_FUNCTION(libvirt_logfile_set)
> RETURN_TRUE;
> }
> #endif
> -
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list