[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 6/6] streams: Report errors if sendAll/recvAll callbacks fail



On Thu, Jun 01, 2017 at 02:23:30PM +0200, Martin Kletzander wrote:
> On Thu, Jun 01, 2017 at 01:10:45PM +0100, Daniel P. Berrange wrote:
> > On Thu, Jun 01, 2017 at 02:02:14PM +0200, Michal Privoznik wrote:
> > > We have couple of wrappers over our low level stream APIs:
> > > virSreamRecvAll(), virStreamSendAll() and their sparse stream
> > > variants. All of them take some callbacks and call them at
> > > appropriate times. If a callback fails it aborts the whole
> > > operation. However, if it so happens that the callback fails
> > > without setting any error users are left with very unhelpful
> > > error message:
> > > 
> > >   error: cannot receive data from volume fedora.img
> > >   error: An error occurred, but the cause is unknown
> > > 
> > > One way to avoid it is to report error if callback hasn't.
> > 
> > The callbacks should never be expected to set a libvirt
> > error, as they are implemented by code that is outside
> > libvirt. The intention was really that the callbacks set
> > an errno value on error, since that's all you have access
> > to from application level impl....
> > 
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn redhat com>
> > > ---
> > >  src/libvirt-stream.c | 41 +++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 33 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/src/libvirt-stream.c b/src/libvirt-stream.c
> > > index 1594ed212..efdbc9e44 100644
> > > --- a/src/libvirt-stream.c
> > > +++ b/src/libvirt-stream.c
> > > @@ -596,8 +596,12 @@ virStreamSendAll(virStreamPtr stream,
> > >      for (;;) {
> > >          int got, offset = 0;
> > 
> > For sanity we should set 'errno = 0' here.
> > 
> > >          got = (handler)(stream, bytes, want, opaque);
> > > -        if (got < 0)
> > > +        if (got < 0) {
> > > +            if (!virGetLastError())
> > > +                virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> > > +                               _("send handler failed"));
> > >              goto cleanup;
> > > +        }
> > 
> > ...then this should do
> > 
> > 
> >  if (errno == 0) {
> >      errno = EIO;
> >  }
> >  virReportSystemError(errno, ....)
> > 
> 
> Oh, that's a good idea.  But can we check for both?  The callbacks can
> still call libvirt (to receive from the stream for example) and that
> error could be kept as well.  Not all errors will set errno.  On the
> other hand the application will already know about the problem and can
> deal with it how it wants, but we should also aim at intuitive usage.

Libvirtd code never uses the SendAll/RecvAll functions AFAIK.

The only user of the callbacks which might call other libvirt
(internal) functions is really virsh, but even that is just
calling plain posix functions which set errno (at least until
patch 5 in this series). So I'm not seeing a compelling scenario
where you need to preseve libvirt errors here.

> If we are to expect errno, we need to say that in the documentation,
> really.

Agreed, in fact I thought we already did, but I guess not.

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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]