Re: [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Kevin Wolf]

Am 14.01.2016 um 13:14 hat Daniel P. Berrange geschrieben:
> On Wed, Jan 13, 2016 at 07:42:20PM +0100, Kevin Wolf wrote:
> > Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:
> > > +static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
> > > +                                          size_t offset,
> > > +                                          uint8_t *buf,
> > > +                                          size_t buflen,
> > > +                                          Error **errp,
> > > +                                          void *opaque)
> > > +{
> > > +    BlockDriverState *bs = opaque;
> > > +    BDRVQcow2State *s = bs->opaque;
> > > +    ssize_t ret;
> > > +
> > > +    if ((offset + buflen) > s->fde_header.length) {
> > > +        error_setg_errno(errp, EINVAL,
> > > +                         "Request for data outside of extension header");
> > 
> > error_setg_errno() with a constant errno doesn't look very useful.
> > Better use plain error_setg() in such cases.
> 
> I wasn't too sure - I figured since the block layer seems to
> propagate errno's around alot, that I ought to report an
> errno here, but will happiyl drop it.

error_setg_errno() doesn't keep the error code around for the callers to
inspect, but just adds the error string to the message. And you already
have a much more useful error message than "Invalid argument".

> > > +        return -1;
> > 
> > Here returning -EINVAL could be useful, I'm not sure what your crypto
> > API requires. At least you seem to be returning -errno below and mixing
> > -1 and -errno is probably a bad idea.
> 
> The crypto API doesn't deal with errno's at all - it uses the
> Error object exclusively, so yeah, I can drop it from the
> place below.

Then you could probably just make the function void. I genereally prefer
to use only one mechanism to return errors instead of both an int return
value and an Error** argument, but there are many places in qemu which
use both. So whatever feels right to you.

Kevin