Michael Tokarev <m...@tls.msk.ru> writes: > 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...
You can add mine in addition to Gonglei's. Signed-off-by: Markus Armbruster <arm...@redhat.com>
