6:26 p.m.
From: "Zeeshan Ali (Khattak)" <zeeshanak(a)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
2:51 a.m.
On Thu, Feb 16, 2012 at 02:26:02AM +0200, Zeeshan Ali (Khattak) wrote:
From: "Zeeshan Ali (Khattak)" <zeeshanak(a)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
3:57 a.m.
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(a)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 :|
3:16 p.m.
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
5:01 p.m.
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 :|
5:20 a.m.
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
8:56 a.m.
On Fri, Feb 17, 2012 at 1:20 PM, Christophe Fergeau <cfergeau(a)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
9:02 a.m.
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
6:47 a.m.
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
9:08 a.m.
On Fri, Feb 17, 2012 at 1:01 AM, Daniel P. Berrange <berrange(a)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
9:18 a.m.
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
9:25 a.m.
On Fri, Feb 17, 2012 at 5:18 PM, Christophe Fergeau <cfergeau(a)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
9:38 a.m.
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(a)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
9:56 a.m.
On Fri, Feb 17, 2012 at 5:38 PM, Christophe Fergeau <cfergeau(a)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(a)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
10:19 a.m.
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(a)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
10:31 a.m.
On Fri, Feb 17, 2012 at 6:19 PM, Christophe Fergeau <cfergeau(a)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(a)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
10:53 a.m.
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
4662
days inactive
4663
days old
16 comments
3 participants
participants (3)
-
Christophe Fergeau -
Daniel P. Berrange -
Zeeshan Ali (Khattak)