[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 01/16] block: Add Prealloc
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 01/16] block: Add PreallocMode to BD.bdrv_truncate() |
Date: |
Thu, 23 Mar 2017 15:32:41 +0000 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Wed, Mar 22, 2017 at 05:50:59PM +0100, Max Reitz wrote:
> On 22.03.2017 17:28, Stefan Hajnoczi wrote:
> > On Mon, Mar 20, 2017 at 04:07:16PM +0100, Max Reitz wrote:
> >> On 20.03.2017 11:18, Stefan Hajnoczi wrote:
> >>> On Mon, Mar 13, 2017 at 10:39:46PM +0100, Max Reitz wrote:
> >>>> diff --git a/block/iscsi.c b/block/iscsi.c
> >>>> index ab559a6f71..5d6265c4a6 100644
> >>>> --- a/block/iscsi.c
> >>>> +++ b/block/iscsi.c
> >>>> @@ -2060,11 +2060,16 @@ static void iscsi_reopen_commit(BDRVReopenState
> >>>> *reopen_state)
> >>>> }
> >>>> }
> >>>>
> >>>> -static int iscsi_truncate(BlockDriverState *bs, int64_t offset, Error
> >>>> **errp)
> >>>> +static int iscsi_truncate(BlockDriverState *bs, int64_t offset,
> >>>> + PreallocMode prealloc, Error **errp)
> >>>> {
> >>>> IscsiLun *iscsilun = bs->opaque;
> >>>> Error *local_err = NULL;
> >>>>
> >>>> + if (prealloc != PREALLOC_MODE_OFF) {
> >>>> + return -ENOTSUP;
> >>>> + }
> >>>> +
> >>>> if (iscsilun->type != TYPE_DISK) {
> >>>> return -ENOTSUP;
> >>>> }
> >>>
> >>> Nevermind what I said about adding a BiteSizedTasks entry:
> >>>
> >>> The missing errp usage is not in qemu.git/master yet. Please fix up
> >>> your bdrv_truncate() errp patch to use errp in all cases, e.g.
> >>> error_setg("Unable to truncate non-disk LUN").
> >>
> >> The thing is that I wasn't comfortable doing that for all block drivers.
> >> I mean, I can take another look but I'd rather have vague error messages
> >> ("truncation failed: #{strerror}") than outright wrong ones because I
> >> didn't know what error message to use.
> >>
> >> Of course you could argue that this may probably come out during review
> >> but that implies that every submaintainer for every block driver would
> >> actually come out for review...
> >
> > I'm worried about errp being set in only a subset of error cases.
> >
> > This is likely to cause bugs if callers use if (local_err). Grepping
> > through the codebase I can see instances of:
>
> Yes, but the generic bdrv_truncate() will always set errp if the driver
> hasn't done so.
I prefer consistently setting errp to make patch review easy (no
checking all callers to see how they behave), making it safe to
copy-paste the code elsewhere, etc.
It's safer to be consistent, can produce more detailed error messages,
and the cost of writing error_setg() calls is low. But it's a matter of
style, you've pointed out the code is technically correct right now.
Stefan
signature.asc
Description: PGP signature
[Qemu-devel] [PATCH for-2.10 02/16] block: Add PreallocMode to bdrv_truncate(), Max Reitz, 2017/03/13
[Qemu-devel] [PATCH for-2.10 05/16] block/file-posix: Small fixes in raw_create(), Max Reitz, 2017/03/13
[Qemu-devel] [PATCH for-2.10 03/16] block: Add PreallocMode to blk_truncate(), Max Reitz, 2017/03/13
[Qemu-devel] [PATCH for-2.10 04/16] qemu-img: Expose PreallocMode for resizing, Max Reitz, 2017/03/13