30.10.2014 10:10, Markus Armbruster wrote: [] > I'm afraid the commit message is a bit misleading. Let's examine what > exactly happens. > > dump_iterate() dumps blocks in a loop. Eventually, get_next_block() > returns "no more". We then call dump_completed(). But we neglect to > break the loop! Broken in commit 4c7e251a. > > Because of that, we dump the last block again. This attempts to write > to s->fd, which fails if we're lucky. The error makes dump_iterate() > return unsuccessfully. It's the only way it can ever return. > > Theoretical: if we're not so lucky, something else has opened something > for writing and got the same fd. dump_iterate() then keeps looping, > messing up the something else's output, until a write fails, or the > process mercifully terminates. > > Is this correct? > > If yes, let's use this commit message: > > dump: Fix dump-guest-memory termination and use-after-close > > dump_iterate() dumps blocks in a loop. Eventually, get_next_block() > returns "no more". We then call dump_completed(). But we neglect to > break the loop! Broken in commit 4c7e251a. > > Because of that, we dump the last block again. This attempts to write > to s->fd, which fails if we're lucky. The error makes dump_iterate() > return failure. It's the only way it can ever return. > > Theoretical: if we're not so lucky, something else has opened something > for writing and got the same fd. dump_iterate() then keeps looping, > messing up the something else's output, until a write fails, or the > process mercifully terminates. > > The obvious fix is to restore the return lost in commit 4c7e251a. But > the root cause of the bug is needlessly opaque loop control. Replace it > by a clean do ... while loop. > > This makes the badly chosen return values of get_next_block() more > visible. Cleaning that up is outside the scope of this bug fix. > > You can then add my R-by.
So I'm applying this -- which is your patch and your commit message, and I really wonder why this is Reviewed-by and not Signed-off-by, with your authorship? It really should be... Thanks, /mjt
