From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48678) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJhTq-0006o9-0f for qemu-devel@nongnu.org; Thu, 14 Jan 2016 07:58:23 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJhTo-0005zB-VN for qemu-devel@nongnu.org; Thu, 14 Jan 2016 07:58:21 -0500 Date: Thu, 14 Jan 2016 13:58:12 +0100 From: Kevin Wolf Message-ID: <20160114125812.GD4084@noname.redhat.com> References: <1452624982-19332-1-git-send-email-berrange@redhat.com> <1452624982-19332-11-git-send-email-berrange@redhat.com> <20160113184220.GD4356@noname.redhat.com> <20160114121455.GL910@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160114121455.GL910@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org 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