On Mon, Jul 10, 2017 at 09:36:50PM -0500, Eric Blake wrote:
> On 07/10/2017 03:15 AM, Kashyap Chamarthy wrote:
[...]
> > Signed-off-by: Kashyap Chamarthy <kcham...@redhat.com>
> > Reviewed-by: John Snow <js...@redhat.com>
> > ---
>
> > ---
> > docs/devel/bitmaps.md | 505 ------------------------------------------
> > docs/interop/bitmaps.rst | 555
> > +++++++++++++++++++++++++++++++++++++++++++++++
>
> A shame that git rename detection doesn't see these as the same rough
> contents, but not too bad.
_Should_ it detect?
> I'll just review the new text; if I point out
> something that was pre-existing in the old text, it may be nicer to
> split the cleanups into a separate followup patch, but I'm also okay if
> they go in as part of this patch.
Indeed -- the things you point out further below were already
pre-existing.
Asking out of curiosity: You say it is nicer split because we'll retain
the `git-bisect`-ability? Or just to keep text motion and actual
changes separate? (I think both.)
> My review is less focused on the choices used in representing things in
> rST and on the final display outcome, and more on the text itself
> (grammar and such).
Noted.
>
> > +++ b/docs/interop/bitmaps.rst
> > +
> > +- Dirty bitmaps can be created at any time and attached to any node
> > + (not just complete drives.)
>
> Looks a bit nicer with '.' outside the sub-sentence '()'.
Will fix.
> > +
> > +.. contents::
> > +
> > +Dirty Bitmap Names
> > +------------------
> > +
> > +- A dirty bitmap's name is unique to the node, but bitmaps attached to
> > + different nodes can share the same name.
> > +
> > +- Dirty bitmaps created for internal use by QEMU may be anonymous and
> > + have no name, but any user-created bitmaps may not be. There can be
>
> s/may not be/must have a name/
Will fix.
>
> > +
> > +- This bitmap will have a default granularity that matches the cluster
> > + size of its associated drive, if available, clamped to between [4KiB,
>
> s/clamped to/clamped/
Will fix.
> > + 64KiB]. The current default for qcow2 is 64KiB.
> > +
> > +
>
>
> > +- QMP example response, highlighting one success and one failure:
> > +
> > + - Acknowledgement that the Transaction was accepted and jobs were
> > + launched:
>
> US spelling prefers Acknowledgment, but I think you are okay because of
> UK spelling.
It was John Snow who wrote this document. :-)
I realize that you merely pointed out the difference. And the QEMU
upstream documentation also says they accept both US and UK spellings:
http://wiki.qemu.org/Contribute/SpellCheck
> Again, since these are probably pre-existing and just code motion, and
> if you've gotten positive review of your use of rST, I'm fine with adding:
>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
Thank you.
I'll wait for feedback on the other (much longer and completely
overhauled) document -- if there's a need to respin it, I'll address
your feedback from here.
[...]
--
/kashyap
