[libvirt] [PATCH 1/2] python: treat flags as default argument with value 0
The following four functions have not changed because default arguments have to come after positional arguments. Changing them will break the the binding APIs. migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth): --- python/generator.py | 2 ++ python/libvirt-override-virConnect.py | 14 +++++++------- python/libvirt-override-virDomain.py | 2 +- python/libvirt-override-virDomainSnapshot.py | 2 +- python/libvirt-override-virStoragePool.py | 2 +- python/libvirt-override.py | 2 +- 6 files changed, 13 insertions(+), 11 deletions(-) diff --git a/python/generator.py b/python/generator.py index d269e88..bb53fcf 100755 --- a/python/generator.py +++ b/python/generator.py @@ -1487,6 +1487,8 @@ def buildWrappers(module): if n != index: classes.write(", %s" % arg[0]) n = n + 1 + if arg[0] == "flags": + classes.write("=0"); classes.write("):\n") writeDoc(module, name, args, ' ', classes) n = 0 diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 121ef6f..5495b70 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -204,7 +204,7 @@ self.domainEventCallbackID[ret] = opaque return ret - def listAllDomains(self, flags): + def listAllDomains(self, flags=0): """List all domains and returns a list of domain objects""" ret = libvirtmod.virConnectListAllDomains(self._o, flags) if ret is None: @@ -216,7 +216,7 @@ return retlist - def listAllStoragePools(self, flags): + def listAllStoragePools(self, flags=0): """Returns a list of storage pool objects""" ret = libvirtmod.virConnectListAllStoragePools(self._o, flags) if ret is None: @@ -228,7 +228,7 @@ return retlist - def listAllNetworks(self, flags): + def listAllNetworks(self, flags=0): """Returns a list of network objects""" ret = libvirtmod.virConnectListAllNetworks(self._o, flags) if ret is None: @@ -240,7 +240,7 @@ return retlist - def listAllInterfaces(self, flags): + def listAllInterfaces(self, flags=0): """Returns a list of interface objects""" ret = libvirtmod.virConnectListAllInterfaces(self._o, flags) if ret is None: @@ -252,7 +252,7 @@ return retlist - def listAllDevices(self, flags): + def listAllDevices(self, flags=0): """Returns a list of host node device objects""" ret = libvirtmod.virConnectListAllNodeDevices(self._o, flags) if ret is None: @@ -264,7 +264,7 @@ return retlist - def listAllNWFilters(self, flags): + def listAllNWFilters(self, flags=0): """Returns a list of network filter objects""" ret = libvirtmod.virConnectListAllNWFilters(self._o, flags) if ret is None: @@ -276,7 +276,7 @@ return retlist - def listAllSecrets(self, flags): + def listAllSecrets(self, flags=0): """Returns a list of secret objects""" ret = libvirtmod.virConnectListAllSecrets(self._o, flags) if ret is None: diff --git a/python/libvirt-override-virDomain.py b/python/libvirt-override-virDomain.py index ccc4d5f..142b1d4 100644 --- a/python/libvirt-override-virDomain.py +++ b/python/libvirt-override-virDomain.py @@ -1,4 +1,4 @@ - def listAllSnapshots(self, flags): + def listAllSnapshots(self, flags=0): """List all snapshots and returns a list of snapshot objects""" ret = libvirtmod.virDomainListAllSnapshots(self._o, flags) if ret is None: diff --git a/python/libvirt-override-virDomainSnapshot.py b/python/libvirt-override-virDomainSnapshot.py index bf708a5..ec53358 100644 --- a/python/libvirt-override-virDomainSnapshot.py +++ b/python/libvirt-override-virDomainSnapshot.py @@ -6,7 +6,7 @@ """Get the domain that a snapshot was created for""" return self.domain() - def listAllChildren(self, flags): + def listAllChildren(self, flags=0): """List all child snapshots and returns a list of snapshot objects""" ret = libvirtmod.virDomainSnapshotListAllChildren(self._o, flags) if ret is None: diff --git a/python/libvirt-override-virStoragePool.py b/python/libvirt-override-virStoragePool.py index ffe160c..325e403 100644 --- a/python/libvirt-override-virStoragePool.py +++ b/python/libvirt-override-virStoragePool.py @@ -1,4 +1,4 @@ - def listAllVolumes(self, flags): + def listAllVolumes(self, flags=0): """List all storage volumes and returns a list of storage volume objects""" ret = libvirtmod.virStoragePoolListAllVolumes(self._o, flags) if ret is None: diff --git a/python/libvirt-override.py b/python/libvirt-override.py index 82d7dcb..ccfec48 100644 --- a/python/libvirt-override.py +++ b/python/libvirt-override.py @@ -85,7 +85,7 @@ def registerErrorHandler(f, ctx): Returns 1 in case of success.""" return libvirtmod.virRegisterErrorHandler(f,ctx) -def openAuth(uri, auth, flags): +def openAuth(uri, auth, flags=0): ret = libvirtmod.virConnectOpenAuth(uri, auth, flags) if ret is None:raise libvirtError('virConnectOpenAuth() failed') return virConnect(_obj=ret) -- 1.7.11.2
--- python/Makefile.am | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/python/Makefile.am b/python/Makefile.am index 55c5e41..18da9a2 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -26,7 +26,8 @@ CLASSES_EXTRA = \ libvirt-override-virConnect.py \ libvirt-override-virDomain.py \ libvirt-override-virDomainSnapshot.py \ - libvirt-override-virStream.py + libvirt-override-virStream.py \ + libvirt-override-virStoragePool.py EXTRA_DIST = \ generator.py \ @@ -109,7 +110,11 @@ LXC_GENERATED= libvirt-lxc-export.c \ libvirt-lxc.h \ libvirt_lxc.py -$(GENERATE).stamp: $(srcdir)/$(GENERATE) $(API_DESC) $(QEMU_API_DESC) $(LXC_API_DESC) +$(GENERATE).stamp: $(srcdir)/$(GENERATE) \ + $(API_DESC) \ + $(QEMU_API_DESC) \ + $(LXC_API_DESC) \ + $(CLASSES_EXTRA) $(AM_V_GEN)$(PYTHON) $(srcdir)/$(GENERATE) $(PYTHON) && \ touch $@ -- 1.7.11.2
On 2013年03月21日 16:41, Guannan Ren wrote:
--- python/Makefile.am | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/python/Makefile.am b/python/Makefile.am index 55c5e41..18da9a2 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -26,7 +26,8 @@ CLASSES_EXTRA = \ libvirt-override-virConnect.py \ libvirt-override-virDomain.py \ libvirt-override-virDomainSnapshot.py \ - libvirt-override-virStream.py + libvirt-override-virStream.py \ + libvirt-override-virStoragePool.py
ACK to this.
EXTRA_DIST = \ generator.py \ @@ -109,7 +110,11 @@ LXC_GENERATED= libvirt-lxc-export.c \ libvirt-lxc.h \ libvirt_lxc.py
-$(GENERATE).stamp: $(srcdir)/$(GENERATE) $(API_DESC) $(QEMU_API_DESC) $(LXC_API_DESC) +$(GENERATE).stamp: $(srcdir)/$(GENERATE) \ + $(API_DESC) \ + $(QEMU_API_DESC) \ + $(LXC_API_DESC) \ + $(CLASSES_EXTRA)
Why do we need to add the manually created files here?
$(AM_V_GEN)$(PYTHON) $(srcdir)/$(GENERATE) $(PYTHON)&& \ touch $@
On 03/21/2013 06:48 PM, Osier Yang wrote:
On 2013年03月21日 16:41, Guannan Ren wrote:
--- python/Makefile.am | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/python/Makefile.am b/python/Makefile.am index 55c5e41..18da9a2 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -26,7 +26,8 @@ CLASSES_EXTRA = \ libvirt-override-virConnect.py \ libvirt-override-virDomain.py \ libvirt-override-virDomainSnapshot.py \ - libvirt-override-virStream.py + libvirt-override-virStream.py \ + libvirt-override-virStoragePool.py
ACK to this.
EXTRA_DIST = \ generator.py \ @@ -109,7 +110,11 @@ LXC_GENERATED= libvirt-lxc-export.c \ libvirt-lxc.h \ libvirt_lxc.py
-$(GENERATE).stamp: $(srcdir)/$(GENERATE) $(API_DESC) $(QEMU_API_DESC) $(LXC_API_DESC) +$(GENERATE).stamp: $(srcdir)/$(GENERATE) \ + $(API_DESC) \ + $(QEMU_API_DESC) \ + $(LXC_API_DESC) \ + $(CLASSES_EXTRA)
Why do we need to add the manually created files here?
The problem here is that after we edit or make some changes to these manually-created files, then, run make, the libvirt.py wrapper file is not be updated. The fix here is to update the libvirt.py by running that recipe again, so adding $(CLASS_EXTRA) to one of that stamp file's prerequisites.
On 03/21/2013 02:41 AM, Guannan Ren wrote:
--- python/Makefile.am | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/python/Makefile.am b/python/Makefile.am index 55c5e41..18da9a2 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -26,7 +26,8 @@ CLASSES_EXTRA = \ libvirt-override-virConnect.py \ libvirt-override-virDomain.py \ libvirt-override-virDomainSnapshot.py \ - libvirt-override-virStream.py + libvirt-override-virStream.py \ + libvirt-override-virStoragePool.py
Not alphabetically sorted.
EXTRA_DIST = \ generator.py \ @@ -109,7 +110,11 @@ LXC_GENERATED= libvirt-lxc-export.c \ libvirt-lxc.h \ libvirt_lxc.py
-$(GENERATE).stamp: $(srcdir)/$(GENERATE) $(API_DESC) $(QEMU_API_DESC) $(LXC_API_DESC) +$(GENERATE).stamp: $(srcdir)/$(GENERATE) \ + $(API_DESC) \ + $(QEMU_API_DESC) \ + $(LXC_API_DESC) \ + $(CLASSES_EXTRA) $(AM_V_GEN)$(PYTHON) $(srcdir)/$(GENERATE) $(PYTHON) && \ touch $@
ACK if you fix sorting. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 2013年03月21日 16:41, Guannan Ren wrote:
The following four functions have not changed because default arguments have to come after positional arguments. Changing them will break the the binding APIs.
migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth):
So how are they filtered? ...
--- python/generator.py | 2 ++ python/libvirt-override-virConnect.py | 14 +++++++------- python/libvirt-override-virDomain.py | 2 +- python/libvirt-override-virDomainSnapshot.py | 2 +- python/libvirt-override-virStoragePool.py | 2 +- python/libvirt-override.py | 2 +- 6 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/python/generator.py b/python/generator.py index d269e88..bb53fcf 100755 --- a/python/generator.py +++ b/python/generator.py @@ -1487,6 +1487,8 @@ def buildWrappers(module): if n != index: classes.write(", %s" % arg[0]) n = n + 1 + if arg[0] == "flags": + classes.write("=0");
...As I see you write "flags=0" for all the automatically generated APIs here? And is there any risk to have other APIs of which flags doesn't default to 0? Except the ones you mentioned in commit log.
classes.write("):\n") writeDoc(module, name, args, ' ', classes) n = 0 diff --git a/python/libvirt-override-virConnect.py b/python/libvirt-override-virConnect.py index 121ef6f..5495b70 100644 --- a/python/libvirt-override-virConnect.py +++ b/python/libvirt-override-virConnect.py @@ -204,7 +204,7 @@ self.domainEventCallbackID[ret] = opaque return ret
- def listAllDomains(self, flags): + def listAllDomains(self, flags=0):
All the left are manually created files. So it's safe.
On 03/21/2013 04:47 AM, Osier Yang wrote:
On 2013年03月21日 16:41, Guannan Ren wrote:
The following four functions have not changed because default arguments have to come after positional arguments. Changing them will break the the binding APIs.
migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth):
So how are they filtered? ...
They are not generated; this patch adds a default to all generated functions, then adds a default to all hand-written functions except for those four.
All the left are manually created files. So it's safe.
I agree, and since I had been asking for this, ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/21/2013 06:47 PM, Osier Yang wrote:
On 2013年03月21日 16:41, Guannan Ren wrote:
The following four functions have not changed because default arguments have to come after positional arguments. Changing them will break the the binding APIs.
migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth):
So how are they filtered? ...
If we add flags=0 to above four APIs, we have to move the flags arguments to the last position in the arguments list because the rule "default arguments have to come after positional arguments." Changing them will break the binding APIs. so I didn't touch them.
--- python/generator.py | 2 ++ python/libvirt-override-virConnect.py | 14 +++++++------- python/libvirt-override-virDomain.py | 2 +- python/libvirt-override-virDomainSnapshot.py | 2 +- python/libvirt-override-virStoragePool.py | 2 +- python/libvirt-override.py | 2 +- 6 files changed, 13 insertions(+), 11 deletions(-)
diff --git a/python/generator.py b/python/generator.py index d269e88..bb53fcf 100755 --- a/python/generator.py +++ b/python/generator.py @@ -1487,6 +1487,8 @@ def buildWrappers(module): if n != index: classes.write(", %s" % arg[0]) n = n + 1 + if arg[0] == "flags": + classes.write("=0");
...As I see you write "flags=0" for all the automatically generated APIs here? And is there any risk to have other APIs of which flags doesn't default to 0? Except the ones you mentioned in commit log.
Yes, I am not sure if the 0 is appropriatefor every APIs. I need more advice here. According to my test, they can accept the 0 value all.
On 03/21/2013 09:24 PM, Guannan Ren wrote:
migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth):
So how are they filtered? ...
If we add flags=0 to above four APIs, we have to move the flags arguments to the last position in the arguments list because the rule "default arguments have to come after positional arguments." Changing them will break the binding APIs. so I didn't touch them.
Actually, we should probably use migrate(self, dconn, flags=0, dname=None, uri=None, bandwidth=0) with sane defaults for all arguments after the flags. After all, the C api states: * virDomainMigrate: * @domain: a domain object * @dconn: destination host (a connection object) * @flags: bitwise-OR of virDomainMigrateFlags * @dname: (optional) rename domain to this at destination * @uri: (optional) dest hostname/URI as seen from the source host * @bandwidth: (optional) specify migration bandwidth limit in Mbps But I'm okay if you change the migrate* functions in a separate patch, since it will be touching more than just flags.
...As I see you write "flags=0" for all the automatically generated APIs here? And is there any risk to have other APIs of which flags doesn't default to 0? Except the ones you mentioned in commit log.
Yes, I am not sure if the 0 is appropriatefor every APIs. I need more advice here. According to my test, they can accept the 0 value all.
flags == 0 should be sane for all APIs that we add. In fact, for many APIs, flags == 0 is the only value that we actually support, when we haven't yet used any flags. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 03/22/2013 11:31 AM, Eric Blake wrote:
On 03/21/2013 09:24 PM, Guannan Ren wrote:
migrate(self, dconn, flags, dname, uri, bandwidth): migrate2(self, dconn, dxml, flags, dname, uri, bandwidth): migrateToURI(self, duri, flags, dname, bandwidth): migrateToURI2(self, dconnuri, miguri, dxml, flags, dname, bandwidth): So how are they filtered? ...
If we add flags=0 to above four APIs, we have to move the flags arguments to the last position in the arguments list because the rule "default arguments have to come after positional arguments." Changing them will break the binding APIs. so I didn't touch them. Actually, we should probably use
migrate(self, dconn, flags=0, dname=None, uri=None, bandwidth=0)
with sane defaults for all arguments after the flags. After all, the C api states:
* virDomainMigrate: * @domain: a domain object * @dconn: destination host (a connection object) * @flags: bitwise-OR of virDomainMigrateFlags * @dname: (optional) rename domain to this at destination * @uri: (optional) dest hostname/URI as seen from the source host * @bandwidth: (optional) specify migration bandwidth limit in Mbps
But I'm okay if you change the migrate* functions in a separate patch, since it will be touching more than just flags.
okay.
...As I see you write "flags=0" for all the automatically generated APIs here? And is there any risk to have other APIs of which flags doesn't default to 0? Except the ones you mentioned in commit log.
Yes, I am not sure if the 0 is appropriatefor every APIs. I need more advice here. According to my test, they can accept the 0 value all. flags == 0 should be sane for all APIs that we add. In fact, for many APIs, flags == 0 is the only value that we actually support, when we haven't yet used any flags.
Okay, thank you guys for the review. I Pushed. Guannan
participants (3)
-
Eric Blake -
Guannan Ren -
Osier Yang