[libvirt] [PATCH C#] Always close connections and free domains
Also free the unmanaged authDataPtr in the virConnectOpenAuth examples. --- examples/MonoDevelop/virConnectOpen/MainWindow.cs | 10 ++++++++++ .../MonoDevelop/virConnectOpenAuth/MainWindow.cs | 20 +++++++++++++------- examples/VisualStudio/virConnectOpen/Form1.cs | 10 ++++++++++ examples/VisualStudio/virConnectOpenAuth/Form1.cs | 16 +++++++++++----- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/examples/MonoDevelop/virConnectOpen/MainWindow.cs b/examples/MonoDevelop/virConnectOpen/MainWindow.cs index 849f99f..0abae49 100644 --- a/examples/MonoDevelop/virConnectOpen/MainWindow.cs +++ b/examples/MonoDevelop/virConnectOpen/MainWindow.cs @@ -25,19 +25,29 @@ public partial class MainWindow : Gtk.Window protected virtual void OnButton1Clicked (object sender, System.EventArgs e) { + StoragePoolListStore.Clear (); + IntPtr conn = Connect.Open(entry1.Text); if (conn != IntPtr.Zero) { int numOfStoragePools = Connect.NumOfStoragePools(conn); if (numOfStoragePools == -1) + { ShowError("Unable to get the number of storage pools"); + goto cleanup; + } string[] storagePoolsNames = new string[numOfStoragePools]; int listStoragePools = Connect.ListStoragePools(conn, ref storagePoolsNames, numOfStoragePools); if (listStoragePools == -1) + { ShowError("Unable to list storage pools"); + goto cleanup; + } foreach (string storagePoolName in storagePoolsNames) AddStoragePoolInTreeView(storagePoolName); + cleanup: + Connect.Close(conn); } else { diff --git a/examples/MonoDevelop/virConnectOpenAuth/MainWindow.cs b/examples/MonoDevelop/virConnectOpenAuth/MainWindow.cs index 996a417..d02acd4 100644 --- a/examples/MonoDevelop/virConnectOpenAuth/MainWindow.cs +++ b/examples/MonoDevelop/virConnectOpenAuth/MainWindow.cs @@ -43,8 +43,12 @@ public partial class MainWindow : Gtk.Window Application.Quit (); a.RetVal = true; } + protected virtual void OnButton1Clicked (object sender, System.EventArgs e) { + // Remove all items + domainListStore.Clear (); + // Fill a structure to pass username and password to callbacks AuthData authData = new AuthData { password = entry3.Text, user_name = entry2.Text }; IntPtr authDataPtr = Marshal.AllocHGlobal(Marshal.SizeOf(authData)); @@ -64,7 +68,8 @@ public partial class MainWindow : Gtk.Window // Request the connection IntPtr conn = Connect.OpenAuth(entry1.Text, ref auth, 0); - + Marshal.FreeHGlobal(authDataPtr); + if (conn != IntPtr.Zero) { // Get the number of defined (not running) domains @@ -73,7 +78,7 @@ public partial class MainWindow : Gtk.Window if (Connect.ListDefinedDomains(conn, ref definedDomainNames, numOfDefinedDomains) == -1) { ShowError("Unable to list defined domains"); - return; + goto cleanup; } // Add the domain names to the listbox @@ -88,7 +93,7 @@ public partial class MainWindow : Gtk.Window if (Connect.ListDomains(conn, runningDomainIDs, numOfRunningDomain) == -1) { ShowError("Unable to list running domains"); - return; + goto cleanup; } // Add the domain names to the listbox @@ -98,16 +103,19 @@ public partial class MainWindow : Gtk.Window if (domainPtr == IntPtr.Zero) { ShowError("Unable to lookup domains by id"); - return; + goto cleanup; } string domainName = Domain.GetName(domainPtr); + Domain.Free(domainPtr); if (string.IsNullOrEmpty(domainName)) { ShowError("Unable to get domain name"); - return; + goto cleanup; } AddDomainInTreeView(domainName); } + + cleanup: Connect.Close(conn); } else @@ -156,7 +164,5 @@ public partial class MainWindow : Gtk.Window } return 0; } - - } diff --git a/examples/VisualStudio/virConnectOpen/Form1.cs b/examples/VisualStudio/virConnectOpen/Form1.cs index 870b83d..240aaaa 100644 --- a/examples/VisualStudio/virConnectOpen/Form1.cs +++ b/examples/VisualStudio/virConnectOpen/Form1.cs @@ -27,18 +27,28 @@ namespace virConnectOpen private void btnConnect_Click(object sender, EventArgs e) { + lbStoragePool.Items.Clear(); + IntPtr conn = Connect.Open(tbURI.Text); if (conn != IntPtr.Zero) { int numOfStoragePools = Connect.NumOfStoragePools(conn); if (numOfStoragePools == -1) + { ShowError("Unable to get the number of storage pools"); + goto cleanup; + } string[] storagePoolsNames = new string[numOfStoragePools]; int listStoragePools = Connect.ListStoragePools(conn, ref storagePoolsNames, numOfStoragePools); if (listStoragePools == -1) + { ShowError("Unable to list storage pools"); + goto cleanup; + } foreach (string storagePoolName in storagePoolsNames) lbStoragePool.Items.Add(storagePoolName); + cleanup: + Connect.Close(conn); } else { diff --git a/examples/VisualStudio/virConnectOpenAuth/Form1.cs b/examples/VisualStudio/virConnectOpenAuth/Form1.cs index 7901c27..87634a0 100644 --- a/examples/VisualStudio/virConnectOpenAuth/Form1.cs +++ b/examples/VisualStudio/virConnectOpenAuth/Form1.cs @@ -44,6 +44,8 @@ namespace virConnectOpenAuth private void btnConnect_Click(object sender, EventArgs e) { + lbDomains.Items.Clear(); + // Fill a structure to pass username and password to callbacks AuthData authData = new AuthData { password = tbPassword.Text, user_name = tbUsername.Text }; IntPtr authDataPtr = Marshal.AllocHGlobal(Marshal.SizeOf(authData)); @@ -62,6 +64,7 @@ namespace virConnectOpenAuth // Request the connection IntPtr conn = Connect.OpenAuth(tbURI.Text, ref auth, 0); + Marshal.FreeHGlobal(authDataPtr); if (conn != IntPtr.Zero) { @@ -70,9 +73,9 @@ namespace virConnectOpenAuth string[] definedDomainNames = new string[numOfDefinedDomains]; if (Connect.ListDefinedDomains(conn, ref definedDomainNames, numOfDefinedDomains) == -1) { - MessageBox.Show("Unable to list defined domains", "List defined domains failed", MessageBoxButtons.OK, + MessageBox.Show("Unable to list defined domains", "List defined domains failed", MessageBoxButtons.OK, MessageBoxIcon.Error); - return; + goto cleanup; } // Add the domain names to the listbox @@ -86,7 +89,7 @@ namespace virConnectOpenAuth { MessageBox.Show("Unable to list running domains", "List running domains failed", MessageBoxButtons.OK, MessageBoxIcon.Error); - return; + goto cleanup; } // Add the domain names to the listbox @@ -97,17 +100,20 @@ namespace virConnectOpenAuth { MessageBox.Show("Unable to lookup domains by id", "Lookup domain failed", MessageBoxButtons.OK, MessageBoxIcon.Error); - return; + goto cleanup; } string domainName = Domain.GetName(domainPtr); + Domain.Free(domainPtr); if (string.IsNullOrEmpty(domainName)) { MessageBox.Show("Unable to get domain name", "Get domain name failed", MessageBoxButtons.OK, MessageBoxIcon.Error); - return; + goto cleanup; } lbDomains.Items.Add(domainName); } + + cleanup: Connect.Close(conn); } else -- 1.7.0.4
On Thu, Oct 28, 2010 at 12:37:18PM +0200, Matthias Bolte wrote:
Also free the unmanaged authDataPtr in the virConnectOpenAuth examples.
I'm no C# expert by all mean ... so I'm not sure I should comment [...]
} string domainName = Domain.GetName(domainPtr); + Domain.Free(domainPtr);
but that I wonder ... When people are used to GC'ed languages, they hate when they have to do the cleanup themselve and end up with leaks left and right (from experience with libxml2 python bindings...) Is there really no way to do some kind of automatic garbage collection of Domain objects, or is adding this just a way to speed up the GC in a loop ? otherwise patch looks 'normal' to my unexercized eye :-) Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
2010/10/28 Daniel Veillard <veillard@redhat.com>:
On Thu, Oct 28, 2010 at 12:37:18PM +0200, Matthias Bolte wrote:
Also free the unmanaged authDataPtr in the virConnectOpenAuth examples.
I'm no C# expert by all mean ... so I'm not sure I should comment
Same here. I'm basically applied my C++ and Java knowledge onto C# and see how far I come with that :)
[...]
} string domainName = Domain.GetName(domainPtr); + Domain.Free(domainPtr);
but that I wonder ... When people are used to GC'ed languages, they hate when they have to do the cleanup themselve and end up with leaks left and right (from experience with libxml2 python bindings...) Is there really no way to do some kind of automatic garbage collection of Domain objects, or is adding this just a way to speed up the GC in a loop ?
otherwise patch looks 'normal' to my unexercized eye :-)
Daniel
Currently the bindings are just a straightforward mapping for the C API into C# using static methods and "void" pointers (actually IntPtr). This basically works, but probably doesn't feel very C#'ish. I think we might complete the bindings at this level. Once that basic thin layer is done and all the hard problems are solved - like the heap issue in the auth callback (has a workaround) or the integer width on different platforms problem with the virDomainInfo struct - then we can wrap this "first level" bindings into a more C#'ish way with instances of the Domain class representing an underlying virDomainPtr etc. At that level we can use the GC and let it collect domain objects. But we'll still keep the explicit Free() methods as in the Java bindings, because I think that the C# GC - as most/all GC languages - doesn't guarantee calling finalizers/destructors at all and we might end up with leaked underlying objects. Maybe I'm wrong here, I'm not a C# expert after all :) Matthias
Hmm, that's what I call a "wrapper" (I don't know if it is the good term) I have created one for my project DAVIM. It is almost complete, I have encapsulate IntPtr and all of this in classes. for example I have class "LibvirtHost" and "LibvirtDomain" and "LibvirtNetwork" and so on. Each object keep his IntPtr and expose smooth methods to use bindings. I don't know if it can be a start point. Arnaud -------------------------------------------------- From: "Matthias Bolte" <matthias.bolte@googlemail.com> Sent: Friday, October 29, 2010 4:58 PM To: <veillard@redhat.com> Cc: <libvir-list@redhat.com>; <arnaud.champion@devatom.fr> Subject: Re: [libvirt] [PATCH C#] Always close connections and free domains
2010/10/28 Daniel Veillard <veillard@redhat.com>:
On Thu, Oct 28, 2010 at 12:37:18PM +0200, Matthias Bolte wrote:
Also free the unmanaged authDataPtr in the virConnectOpenAuth examples.
I'm no C# expert by all mean ... so I'm not sure I should comment
Same here. I'm basically applied my C++ and Java knowledge onto C# and see how far I come with that :)
[...]
} string domainName = Domain.GetName(domainPtr); + Domain.Free(domainPtr);
but that I wonder ... When people are used to GC'ed languages, they hate when they have to do the cleanup themselve and end up with leaks left and right (from experience with libxml2 python bindings...) Is there really no way to do some kind of automatic garbage collection of Domain objects, or is adding this just a way to speed up the GC in a loop ?
otherwise patch looks 'normal' to my unexercized eye :-)
Daniel
Currently the bindings are just a straightforward mapping for the C API into C# using static methods and "void" pointers (actually IntPtr). This basically works, but probably doesn't feel very C#'ish.
I think we might complete the bindings at this level. Once that basic thin layer is done and all the hard problems are solved - like the heap issue in the auth callback (has a workaround) or the integer width on different platforms problem with the virDomainInfo struct - then we can wrap this "first level" bindings into a more C#'ish way with instances of the Domain class representing an underlying virDomainPtr etc.
At that level we can use the GC and let it collect domain objects. But we'll still keep the explicit Free() methods as in the Java bindings, because I think that the C# GC - as most/all GC languages - doesn't guarantee calling finalizers/destructors at all and we might end up with leaked underlying objects. Maybe I'm wrong here, I'm not a C# expert after all :)
Matthias
On 10/30/2010 02:06 AM, arnaud.champion@devatom.fr wrote:
Hmm, that's what I call a "wrapper" (I don't know if it is the good term)
I have created one for my project DAVIM. It is almost complete, I have encapsulate IntPtr and all of this in classes.
for example I have class "LibvirtHost" and "LibvirtDomain" and "LibvirtNetwork" and so on. Each object keep his IntPtr and expose smooth methods to use bindings.
I don't know if it can be a start point.
Sounds like it might be worth looking at later on, after we get these initial C# pieces working nicely. :)
participants (4)
-
arnaud.champion＠devatom.fr -
Daniel Veillard -
Justin Clift -
Matthias Bolte