[libvirt] [libvirt-glib] Add gvir_domain_get_saved()
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org> Binding for virDomainHasManagedSaveImage(). --- libvirt-gobject/libvirt-gobject-domain.c | 13 +++++++++++++ libvirt-gobject/libvirt-gobject-domain.h | 1 + libvirt-gobject/libvirt-gobject.sym | 1 + 3 files changed, 15 insertions(+), 0 deletions(-) diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index d9e4c00..5f26dcd 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -854,3 +854,16 @@ gboolean gvir_domain_get_persistent(GVirDomain *dom) return virDomainIsPersistent(dom->priv->handle) == 1; } + +/** + * gvir_domain_get_saved: + * @dom: the domain + * + * Returns: TRUE if domain is in a saved state, FALSE otherwise. + */ +gboolean gvir_domain_get_saved(GVirDomain *dom) +{ + g_return_val_if_fail(GVIR_IS_DOMAIN(dom), FALSE); + + return virDomainHasManagedSaveImage(dom->priv->handle, 0) == 1; +} diff --git a/libvirt-gobject/libvirt-gobject-domain.h b/libvirt-gobject/libvirt-gobject-domain.h index 20388f2..bdff32e 100644 --- a/libvirt-gobject/libvirt-gobject-domain.h +++ b/libvirt-gobject/libvirt-gobject-domain.h @@ -167,6 +167,7 @@ gboolean gvir_domain_save_finish (GVirDomain *dom, GAsyncResult *result, GError **err); gboolean gvir_domain_get_persistent(GVirDomain *dom); +gboolean gvir_domain_get_saved(GVirDomain *dom); G_END_DECLS diff --git a/libvirt-gobject/libvirt-gobject.sym b/libvirt-gobject/libvirt-gobject.sym index 468bf65..7a2f65d 100644 --- a/libvirt-gobject/libvirt-gobject.sym +++ b/libvirt-gobject/libvirt-gobject.sym @@ -64,6 +64,7 @@ LIBVIRT_GOBJECT_0.0.4 { gvir_domain_set_config; gvir_domain_get_info; gvir_domain_get_persistent; + gvir_domain_get_saved; gvir_domain_screenshot; gvir_domain_snapshot_get_type; -- 1.7.7.6
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Binding for virDomainHasManagedSaveImage(). --- libvirt-gobject/libvirt-gobject-domain.c | 13 +++++++++++++ libvirt-gobject/libvirt-gobject-domain.h | 1 + libvirt-gobject/libvirt-gobject.sym | 1 + 3 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index d9e4c00..5f26dcd 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -854,3 +854,16 @@ gboolean gvir_domain_get_persistent(GVirDomain *dom)
return virDomainIsPersistent(dom->priv->handle) == 1; } + +/** + * gvir_domain_get_saved: + * @dom: the domain + * + * Returns: TRUE if domain is in a saved state, FALSE otherwise. ^^^^^^^^^^^
I'd reword this a bit, when reading this, I'm wondering why it's not in GVirDomainState.
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image(); Christophe
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak@gnome.org>
Binding for virDomainHasManagedSaveImage(). --- libvirt-gobject/libvirt-gobject-domain.c | 13 +++++++++++++ libvirt-gobject/libvirt-gobject-domain.h | 1 + libvirt-gobject/libvirt-gobject.sym | 1 + 3 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/libvirt-gobject/libvirt-gobject-domain.c b/libvirt-gobject/libvirt-gobject-domain.c index d9e4c00..5f26dcd 100644 --- a/libvirt-gobject/libvirt-gobject-domain.c +++ b/libvirt-gobject/libvirt-gobject-domain.c @@ -854,3 +854,16 @@ gboolean gvir_domain_get_persistent(GVirDomain *dom)
return virDomainIsPersistent(dom->priv->handle) == 1; } + +/** + * gvir_domain_get_saved: + * @dom: the domain + * + * Returns: TRUE if domain is in a saved state, FALSE otherwise. ^^^^^^^^^^^
I'd reword this a bit, when reading this, I'm wondering why it's not in GVirDomainState.
For historical back compatibility we didn't want to change that. Apps rely on the fact that the 'SHUTOFF' state reflects an inactive domain and all other states reflect an activate domain. Adding a 'SAVED' state would have confused apps. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there? Thanks, Christophe
On Thu, Feb 16, 2012 at 10:16:11PM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there?
Oh, I ACK'd his new patch, without noticing your message in this thread. I tend to agree with your suggestion to rename to gvir_domain_has_managed_save_image() Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, Feb 16, 2012 at 11:01:45PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 16, 2012 at 10:16:11PM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there?
Oh, I ACK'd his new patch, without noticing your message in this thread.
And I had missed the other thread too :) Zeeshan, it's up to the patch submitter to keep track of the comments raised during review, and to make sure all comments are addressed before committing. When several people comment on a patch, you cannot push your changes before reaching a consensus from everyone on the latest version of your patch. You generally have 2 ways of addressing a reviewer comment: * either you agree that his suggested changes improve the code, and you make these changes * either you think your way is better, in which case you explain why you did things this way and why you think this is better. Then, you need to wait and see if the reviewer got convinced to go your way, or if he has more objections/explanations. In which case we begin another iteration. This process helps to get the code and the design of the library as good and consistent as possible. Hope that helps, Christophe
On Fri, Feb 17, 2012 at 1:20 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Thu, Feb 16, 2012 at 11:01:45PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 16, 2012 at 10:16:11PM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there?
Oh, I ACK'd his new patch, without noticing your message in this thread.
And I had missed the other thread too :)
Zeeshan, it's up to the patch submitter to keep track of the comments raised during review,
So sorry, I somehow missed that part of your mail. It was an honest mistake. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Fri, Feb 17, 2012 at 04:56:31PM +0200, Zeeshan Ali (Khattak) wrote:
Zeeshan, it's up to the patch submitter to keep track of the comments raised during review,
So sorry, I somehow missed that part of your mail. It was an honest mistake.
No problem, this happens :) Christophe
On Thu, Feb 16, 2012 at 11:01:45PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 16, 2012 at 10:16:11PM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there?
Oh, I ACK'd his new patch, without noticing your message in this thread.
I tend to agree with your suggestion to rename to
gvir_domain_has_managed_save_image()
Actually, looking more closely at the API, virDomainManagedSave is wrapped in gvir_domain_save, so gvir_domain_has_managed_save_image would be inconsistent. However, _get_saved is a very confusing name for me, gvir_domain_is_saved would fit much better, but we already had this discussion... Christophe
On Fri, Feb 17, 2012 at 1:01 AM, Daniel P. Berrange <berrange@redhat.com> wrote:
On Thu, Feb 16, 2012 at 10:16:11PM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 09:51:34AM +0100, Christophe Fergeau wrote:
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
+ */ +gboolean gvir_domain_get_saved(GVirDomain *dom)
The naming needs to be more explicit, libvirt will suspend the domain after a call to virDomainSave or virDomainManagedSave, the current name only checks for the latter state. I'd go for gvir_domain_has_managed_save_image();
I see this patch has been pushed to master with this part unchanged and no discussion whatsoever on the list, what happened there?
Oh, I ACK'd his new patch, without noticing your message in this thread.
I tend to agree with your suggestion to rename to
gvir_domain_has_managed_save_image()
I disagree. I think we'd want a property for this too and that means we need to keep the 'get' prefix and the string after that should correspond the property name (We already discussed at length why this is needed and we are already doing it for other boolean getters so lets not have the discussion about this need, again). I think gvir_domain_get_saved() might not be descriptive enough but it doesn't cause any confusion either so if its purpose is not clear from the signature, developer will read the (a few words of) docs for it and know what it does. Another reason I don't like the suggested name is that its giving out details that I dont want app developer to care/know about: image. What user needs to care is whether or not the domain in question has a saved state or not that it could be restored to, that is it. He doesn't even need to know what is 'managed' cause we are only providing APIs for 'managed' saving. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Fri, Feb 17, 2012 at 05:08:12PM +0200, Zeeshan Ali (Khattak) wrote:
(We already discussed at length why this is needed and we are already doing it for other boolean getters so lets not have the discussion about this need, again).
Actually this was discussed for libosinfo, not libvirt-glib, here is the relevant email for those who were wondering about this discussion: https://www.redhat.com/archives/virt-tools-list/2011-November/msg00090.html Christophe
On Fri, Feb 17, 2012 at 5:18 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Fri, Feb 17, 2012 at 05:08:12PM +0200, Zeeshan Ali (Khattak) wrote:
(We already discussed at length why this is needed and we are already doing it for other boolean getters so lets not have the discussion about this need, again).
Actually this was discussed for libosinfo, not libvirt-glib, here is the relevant email for those who were wondering about this discussion:
https://www.redhat.com/archives/virt-tools-list/2011-November/msg00090.html
Ah ok but both libraries are meant to be first-class g* citizens and hence the same need to follow the usual conventions unless there is a compelling reason not to. Also what I said about us already following this convention is not incorrect either, e.g gvir_domain_get_persistent() declared/defined just above this new function. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Fri, Feb 17, 2012 at 05:25:30PM +0200, Zeeshan Ali (Khattak) wrote:
On Fri, Feb 17, 2012 at 5:18 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Fri, Feb 17, 2012 at 05:08:12PM +0200, Zeeshan Ali (Khattak) wrote:
(We already discussed at length why this is needed and we are already doing it for other boolean getters so lets not have the discussion about this need, again).
Actually this was discussed for libosinfo, not libvirt-glib, here is the relevant email for those who were wondering about this discussion:
https://www.redhat.com/archives/virt-tools-list/2011-November/msg00090.html
Ah ok but both libraries are meant to be first-class g* citizens and hence the same need to follow the usual conventions unless there is a compelling reason not to.
Making the C API as nice as possible to users is a very compelling reason to me since we are writing a C library (emphasis on the "to me", I know we disagree :) This naming convention for getters is probably only useful for vala, I think bindings for dynamic languages will introspect object properties at runtime and use g_object_get(). So the decision to make is between making the API nicer to read for C users VS making life slightly easier for some bindings. Would a Rename to: annotation help vala here? Or is there some annotation I don't know of to mark property getters/setters? Christophe
On Fri, Feb 17, 2012 at 5:38 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Fri, Feb 17, 2012 at 05:25:30PM +0200, Zeeshan Ali (Khattak) wrote:
On Fri, Feb 17, 2012 at 5:18 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Fri, Feb 17, 2012 at 05:08:12PM +0200, Zeeshan Ali (Khattak) wrote:
(We already discussed at length why this is needed and we are already doing it for other boolean getters so lets not have the discussion about this need, again).
Actually this was discussed for libosinfo, not libvirt-glib, here is the relevant email for those who were wondering about this discussion:
https://www.redhat.com/archives/virt-tools-list/2011-November/msg00090.html
Ah ok but both libraries are meant to be first-class g* citizens and hence the same need to follow the usual conventions unless there is a compelling reason not to.
Making the C API as nice as possible to users is a very compelling reason to me since we are writing a C library (emphasis on the "to me", I know we disagree :)
Indeed we do. :)
This naming convention for getters is probably only useful for vala, I think bindings for dynamic languages will introspect object properties at runtime and use g_object_get().
Well, vala will also do the same but setting properties through that is known to be considerably slower than using the getter/setter directly (because of the type checks etc invovled in case of g_object_get).
So the decision to make is between making the API nicer to read for C users VS making life slightly easier for some bindings.
That is not the decision at all for me since I don't see anyone other than you complaining about the various gtk+ APIs following this convention. If you can cite examples of C developers complaining about it, that would be convincing argument to me. Otherwise, the decision to me is all about following a usual convention *that we already follow* and in turn make valac produce more efficient bindings vs making you happy.
Would a Rename to: annotation help vala here? Or is there some annotation I don't know of to mark property getters/setters?
Maybe? But I don't think we are that desperate yet. :) -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Fri, Feb 17, 2012 at 05:56:19PM +0200, Zeeshan Ali (Khattak) wrote:
On Fri, Feb 17, 2012 at 5:38 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
This naming convention for getters is probably only useful for vala, I think bindings for dynamic languages will introspect object properties at runtime and use g_object_get().
Well, vala will also do the same but setting properties through that is known to be considerably slower than using the getter/setter directly (because of the type checks etc invovled in case of g_object_get).
Yes, and I'm not saying we should go this way.
So the decision to make is between making the API nicer to read for C users VS making life slightly easier for some bindings.
That is not the decision at all for me since I don't see anyone other than you complaining about the various gtk+ APIs following this convention. If you can cite examples of C developers complaining about it, that would be convincing argument to me.
This is such a small annoyance that noone will complain only about it, but the difference between an okayish API and a great API to use lies in all these small details. I'm quite sure I've seen people making fun of _get_has_xxx though. And you also agreed that _is_ was better than _get_ in that email link I gave: "I admit that sounds better", so it's not just me (and I think most people would say _is_xxx is better, and not many people would answer upfront "oh, all g* APIs use get_xxx so you should use this").
Would a Rename to: annotation help vala here? Or is there some annotation I don't know of to mark property getters/setters?
Maybe? But I don't think we are that desperate yet. :)
Why? You are the one insisting that the vala bindings are as efficient as possible, it would be nice to know exactly what options we have. Christophe
On Fri, Feb 17, 2012 at 6:19 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
On Fri, Feb 17, 2012 at 05:56:19PM +0200, Zeeshan Ali (Khattak) wrote:
On Fri, Feb 17, 2012 at 5:38 PM, Christophe Fergeau <cfergeau@redhat.com> wrote:
This naming convention for getters is probably only useful for vala, I think bindings for dynamic languages will introspect object properties at runtime and use g_object_get().
Well, vala will also do the same but setting properties through that is known to be considerably slower than using the getter/setter directly (because of the type checks etc invovled in case of g_object_get).
Yes, and I'm not saying we should go this way.
So the decision to make is between making the API nicer to read for C users VS making life slightly easier for some bindings.
That is not the decision at all for me since I don't see anyone other than you complaining about the various gtk+ APIs following this convention. If you can cite examples of C developers complaining about it, that would be convincing argument to me.
This is such a small annoyance that noone will complain only about it,
Small? And yet we keep discussing this all over again and again in detail? Also you can't know this for certain, maybe most people don't find it annoying at all?
Would a Rename to: annotation help vala here? Or is there some annotation I don't know of to mark property getters/setters?
Maybe? But I don't think we are that desperate yet. :)
Why? You are the one insisting that the vala bindings are as efficient as possible, it would be nice to know exactly what options we have.
By following a usual convention that we are already following and something we have already discussed and we already went with my proposal. If you would be making me go through this each time we add a boolean getter, please go ahead and change the API that way you think is pretty. I won't object at all, as long as you change all the other getters too in both libosinfo and libvirt-glib. Alternatively we can both compromise and agree on 'get_is_saved'. -- Regards, Zeeshan Ali (Khattak) FSF member#5124
On Fri, Feb 17, 2012 at 06:31:31PM +0200, Zeeshan Ali (Khattak) wrote:
This is such a small annoyance that noone will complain only about it,
Small? And yet we keep discussing this all over again and again in detail?
You skipped the rationale as why I think it's small but can be important. And this is the first time we have this discussion for libvirt-glib. My initial concern was that "get_saved" looks weird btw, not that we have to use "is_saved".
By following a usual convention that we are already following and something we have already discussed and we already went with my proposal.
In libosinfo.
If you would be making me go through this each time we add a boolean getter, please go ahead and change the API that way you think is pretty. I won't object at all, as long as you change all the other getters too in both libosinfo and libvirt-glib.
The initial concern was with the naming of the _get_saved function where I indicated my personal preference for an API as good as possible, then this became "shut up, everyone is using _get_saved so it's the only thing that should be considered, this is not even worth mentioning anything else!!". Even the initial name was quite easy to explain given that we want it to be similar to gvir_domain_save, and we want to use _get_ for getters. Which would have been 1) constructive 2) enough to get me to think again about the naming.
Alternatively we can both compromise and agree on 'get_is_saved'.
Both would probably work, not sure which one is less bad. By the way, gtk+ doesn't use _get_ when there is no associated g_object_property (especially with _has_), we really should start adding actual properties and not only getters :) Christophe
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Zeeshan Ali (Khattak)