From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60500) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZtOnZ-0002Y3-Kn for qemu-devel@nongnu.org; Mon, 02 Nov 2015 18:46:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZtOnY-0000lG-OF for qemu-devel@nongnu.org; Mon, 02 Nov 2015 18:46:01 -0500 References: <1446506900-27733-1-git-send-email-jsnow@redhat.com> <5637F525.5060109@redhat.com> From: John Snow Message-ID: <5637F5AE.5020006@redhat.com> Date: Mon, 2 Nov 2015 18:45:50 -0500 MIME-Version: 1.0 In-Reply-To: <5637F525.5060109@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-img: add check for zero-length job len List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-block@nongnu.org Cc: kwolf@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org On 11/02/2015 06:43 PM, Eric Blake wrote: > On 11/02/2015 04:28 PM, John Snow wrote: >> The mirror job doesn't update its total length until >> it has already started running, so we should translate >> a zero-length job-len as meaning 0%. >> >> Otherwise, we may get divide-by-zero faults. >> >> Signed-off-by: John Snow >> --- >> qemu-img.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) > > And indeed, this has tripped up libvirt in the past :) > > My only concern is what if you truly have a 0-length job? For example, > when doing two block-stream commands with identical arguments in a row, > the second block-stream has no work to do, but can complete instantly. > > Will this result in such a job never reporting 100% complete? If so, > that's bad. > A few lines below the context: /* A block job may finish instantaneously without publishing any progress, * so just signal completion here */ qemu_progress_print(100.f, 0); > If you can answer my concerns that we don't have a design bug, then the > code changes look correct, and you can add: > > Reviewed-by: Eric Blake > >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 3025776..38b4888 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -656,7 +656,8 @@ static void run_block_job(BlockJob *job, Error **errp) >> >> do { >> aio_poll(aio_context, true); >> - qemu_progress_print((float)job->offset / job->len * 100.f, 0); >> + qemu_progress_print(job->len ? >> + ((float)job->offset / job->len * 100.f) : 0.00, 0); > > Also, note that this promotes to double rather than float; maybe you > want to use 0.f instead of 0.00 to keep the ternary as a float? But it > shouldn't make a difference in practice. > Yes, oops -- but harmless.