| Message ID | 1263816696-24122-9-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
> #endif > - if (length <= 0) > + if (length < 0) { > return -EINVAL; > + } > + > start = offset & ~(s->cluster_size - 1); > last = (offset + length - 1) & ~(s->cluster_size - 1); > for(cluster_offset = start; cluster_offset <= last; So for legnth = 0, last will equal start and we'll never go through the loop. But should we really bother with all the other work in the function or just return 0 early on?
Am 19.01.2010 19:53, schrieb Christoph Hellwig: >> #endif >> - if (length <= 0) >> + if (length < 0) { >> return -EINVAL; >> + } >> + >> start = offset & ~(s->cluster_size - 1); >> last = (offset + length - 1) & ~(s->cluster_size - 1); >> for(cluster_offset = start; cluster_offset <= last; > > So for legnth = 0, last will equal start and we'll never go through > the loop. But should we really bother with all the other work in the > function or just return 0 early on? I'm not a big fan of special-casing for no real reason ("all the other work" basically is calculating start and last and skipping two ifs - and length = 0 is an unusual case anyway), but if you really mind we can change it. Kevin
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index a84620f..3dfadf1 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -284,8 +284,10 @@ static int update_refcount(BlockDriverState *bs, printf("update_refcount: offset=%" PRId64 " size=%" PRId64 " addend=%d\n", offset, length, addend); #endif - if (length <= 0) + if (length < 0) { return -EINVAL; + } + start = offset & ~(s->cluster_size - 1); last = (offset + length - 1) & ~(s->cluster_size - 1); for(cluster_offset = start; cluster_offset <= last;
There's absolutely no problem with updating the refcounts of 0 clusters. At least snapshot code is doing this and would fail once the result of update_refcount isn't ignored any more. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/qcow2-refcount.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)