In this case we want to deprecate the API wholesale, so we
can simply report a warning in the public entry point.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/libvirt-domain.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index 7690339521..a758539b6d 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -97,6 +97,11 @@ virConnectNumOfDomains(virConnectPtr conn)
int ret = conn->driver->connectNumOfDomains(conn);
if (ret < 0)
goto error;
+
+ virReportWarning(VIR_ERR_DEPRECATED_FEATURE,
+ "%s",
+ "virConnectNumOfDomains()");
+ virDispatchError(conn);
return ret;
}
--
2.17.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 02, 2018 at 16:14:44 +0200, Andrea Bolognani wrote: > In this case we want to deprecate the API wholesale, so we > can simply report a warning in the public entry point. > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > --- > src/libvirt-domain.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > index 7690339521..a758539b6d 100644 > --- a/src/libvirt-domain.c > +++ b/src/libvirt-domain.c > @@ -97,6 +97,11 @@ virConnectNumOfDomains(virConnectPtr conn) > int ret = conn->driver->connectNumOfDomains(conn); > if (ret < 0) > goto error; > + > + virReportWarning(VIR_ERR_DEPRECATED_FEATURE, > + "%s", > + "virConnectNumOfDomains()"); > + virDispatchError(conn); I don't think our API contract allows for reporting an error AND returning success. And even if we didn't specify it it's pretty much assumed right now, so I'm not in support of this. > return ret; > } > > -- > 2.17.1 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, 2018-10-02 at 16:43 +0200, Peter Krempa wrote: > On Tue, Oct 02, 2018 at 16:14:44 +0200, Andrea Bolognani wrote: [...] > > @@ -97,6 +97,11 @@ virConnectNumOfDomains(virConnectPtr conn) > > int ret = conn->driver->connectNumOfDomains(conn); > > if (ret < 0) > > goto error; > > + > > + virReportWarning(VIR_ERR_DEPRECATED_FEATURE, > > + "%s", > > + "virConnectNumOfDomains()"); > > + virDispatchError(conn); > > I don't think our API contract allows for reporting an error AND > returning success. And even if we didn't specify it it's pretty much > assumed right now, so I'm not in support of this. I'm afraid I don't have a very good answer for this concern, as for the most part I share it. This is the best I could come up with on my own; hopefully other developers such as yourself will contribute to the discussion and we'll collectively come up with something much better :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 02, 2018 at 17:25:34 +0200, Andrea Bolognani wrote: > On Tue, 2018-10-02 at 16:43 +0200, Peter Krempa wrote: > > On Tue, Oct 02, 2018 at 16:14:44 +0200, Andrea Bolognani wrote: > [...] > > I don't think our API contract allows for reporting an error AND > > returning success. And even if we didn't specify it it's pretty much > > assumed right now, so I'm not in support of this. > > I'm afraid I don't have a very good answer for this concern, as > for the most part I share it. > > This is the best I could come up with on my own; hopefully other > developers such as yourself will contribute to the discussion and > we'll collectively come up with something much better :) Well, my suggestion is not to do it. That won't contribute much towards the discussion but will greatly contribute to not breaking of the code. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 02, 2018 at 17:25:34 +0200, Andrea Bolognani wrote: > On Tue, 2018-10-02 at 16:43 +0200, Peter Krempa wrote: > > On Tue, Oct 02, 2018 at 16:14:44 +0200, Andrea Bolognani wrote: > [...] > > > @@ -97,6 +97,11 @@ virConnectNumOfDomains(virConnectPtr conn) > > > int ret = conn->driver->connectNumOfDomains(conn); > > > if (ret < 0) > > > goto error; > > > + > > > + virReportWarning(VIR_ERR_DEPRECATED_FEATURE, > > > + "%s", > > > + "virConnectNumOfDomains()"); > > > + virDispatchError(conn); > > > > I don't think our API contract allows for reporting an error AND > > returning success. And even if we didn't specify it it's pretty much > > assumed right now, so I'm not in support of this. > > I'm afraid I don't have a very good answer for this concern, as > for the most part I share it. > > This is the best I could come up with on my own; hopefully other > developers such as yourself will contribute to the discussion and > we'll collectively come up with something much better :) Technically you could introduce a new event and report warnings as events. That would much cleaner IMHO. Of course, apps would have to run an event loop and register for getting such events, but not doing so is not too different from just ignoring them when they are forcefully pushed on clients by some ugly black magic. Your approach only works for stuff that can be detected at a client library level, but useless in case you'd want to deprecate something in an XML, for example. Our RPC can only transfer a success or an error, but not both. A new event would be useful for all cases and you could even report several warning for a single API call. Jirka -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, 2018-10-03 at 13:14 +0200, Jiri Denemark wrote: > On Tue, Oct 02, 2018 at 17:25:34 +0200, Andrea Bolognani wrote: > > This is the best I could come up with on my own; hopefully other > > developers such as yourself will contribute to the discussion and > > we'll collectively come up with something much better :) > > Technically you could introduce a new event and report warnings as > events. That would much cleaner IMHO. Of course, apps would have to run > an event loop and register for getting such events, but not doing so is > not too different from just ignoring them when they are forcefully > pushed on clients by some ugly black magic. Your approach only works for > stuff that can be detected at a client library level, but useless in > case you'd want to deprecate something in an XML, for example. Our RPC > can only transfer a success or an error, but not both. A new event would > be useful for all cases and you could even report several warning for a > single API call. Yeah, I've run into the RPC issue and mention it in the cover letter as something that needs to be addressed before this approach can be realistically considered. The idea of using events to report warnings sounds neat! I wonder much of a limitation the fact that you'd need to have an event loop running would be in practice... How likely is it that someone will write/has written a useful, non-helloworld libvirt application that doesn't have one? Could we detect whether an even loop is running from library code and, much further down the line, refuse to execute commands unless it is? Of course then you get into the fun of warning developers that they're not going to receive warnings :) -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Oct 04, 2018 at 13:13:48 +0200, Andrea Bolognani wrote: > On Wed, 2018-10-03 at 13:14 +0200, Jiri Denemark wrote: > > On Tue, Oct 02, 2018 at 17:25:34 +0200, Andrea Bolognani wrote: > > > This is the best I could come up with on my own; hopefully other > > > developers such as yourself will contribute to the discussion and > > > we'll collectively come up with something much better :) > > > > Technically you could introduce a new event and report warnings as > > events. That would much cleaner IMHO. Of course, apps would have to run > > an event loop and register for getting such events, but not doing so is > > not too different from just ignoring them when they are forcefully > > pushed on clients by some ugly black magic. Your approach only works for > > stuff that can be detected at a client library level, but useless in > > case you'd want to deprecate something in an XML, for example. Our RPC > > can only transfer a success or an error, but not both. A new event would > > be useful for all cases and you could even report several warning for a > > single API call. > > Yeah, I've run into the RPC issue and mention it in the cover letter > as something that needs to be addressed before this approach can be > realistically considered. > > The idea of using events to report warnings sounds neat! I wonder > much of a limitation the fact that you'd need to have an event loop > running would be in practice... > > How likely is it that someone will write/has written a useful, > non-helloworld libvirt application that doesn't have one? Could we > detect whether an even loop is running from library code and, much > further down the line, refuse to execute commands unless it is? So you want to mandate running an event loop? While that would allow removing some complexity from the RPC code it's not possible to do it without breaking apps not running the event loop. > Of course then you get into the fun of warning developers that > they're not going to receive warnings :) I don't think there's a possibility to mandate reception of warnings since it would break apps which would not use it at the time. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Tue, Oct 02, 2018 at 04:43:01PM +0200, Peter Krempa wrote: > On Tue, Oct 02, 2018 at 16:14:44 +0200, Andrea Bolognani wrote: > > In this case we want to deprecate the API wholesale, so we > > can simply report a warning in the public entry point. > > > > Signed-off-by: Andrea Bolognani <abologna@redhat.com> > > --- > > src/libvirt-domain.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c > > index 7690339521..a758539b6d 100644 > > --- a/src/libvirt-domain.c > > +++ b/src/libvirt-domain.c > > @@ -97,6 +97,11 @@ virConnectNumOfDomains(virConnectPtr conn) > > int ret = conn->driver->connectNumOfDomains(conn); > > if (ret < 0) > > goto error; > > + > > + virReportWarning(VIR_ERR_DEPRECATED_FEATURE, > > + "%s", > > + "virConnectNumOfDomains()"); > > + virDispatchError(conn); > > I don't think our API contract allows for reporting an error AND > returning success. And even if we didn't specify it it's pretty much > assumed right now, so I'm not in support of this. Yep, even though our public header has VIR_ERR_WARNING level flag, this is impossible for us to use in practice. It was only ever usable back in the very early days of libvirt, when there was no RPC and you had an error callback function registered. No app ever uses the error callbacks, because it is a single callback per-process making it unusable when a single process has different bits of code all calling libvirt. Meanwhile the RPC code won't transport more than one error object and will only do it when the API fails, so this warning will be lost for most drivers. If anything we should just put a comment in virterror.h that VIR_ERR_WARNING will never be seen, so apps can assume VIR_ERR_ERROR. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2023 Red Hat, Inc.