Re: [PATCH v3] xen/public: move xenstore related doc into 9pfs.h

[Julien Grall <julien@xxxxxxx>]

Hi,

On 30/01/2023 09:23, Juergen Gross wrote:
> On 30.01.23 10:18, Julien Grall wrote:
> > Hi Juergen,
> >
> > On 30/01/2023 09:09, Juergen Gross wrote:
> > > The Xenstore related documentation is currently to be found in
> > > docs/misc/9pfs.pandoc, instead of the related header file
> > > xen/include/public/io/9pfs.h like for most other paravirtualized
> > > device protocols.
> > >
> > > There is a comment in the header pointing at the document, but the
> > > given file name is wrong. Additionally such headers are meant to be
> > > copied into consuming projects (Linux kernel, qemu, etc.), so pointing
> > > at a doc file in the Xen git repository isn't really helpful for the
> > > consumers of the header.
> > >
> > > This situation is far from ideal, which is already being proved by the
> > > fact that neither qemu nor the Linux kernel are implementing the
> > > device attach/detach protocol correctly. Additionally the documented
> > > Xenstore entries are not matching the reality, as the "tag" Xenstore
> > > entry is on the frontend side, not on the backend one.
> > >
> > > There is another bug in the connection sequence: the frontend needs to
> > > wait for the backend to have published its features before being able
> > > to allocate its rings and event-channels.
> > >
> > > Change that by moving the Xenstore related 9pfs documentation from
> > > docs/misc/9pfs.pandoc into xen/include/public/io/9pfs.h while fixing
> > > the wrong Xenstore entry detail and the connection sequence.
> > >
> > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > > ---
> > > V2:
> > > - add reference to header in the pandoc document (Jan Beulich)
> > > V3:
> > > - fix flaw in the connection sequence
> > Please don't do that in the same patch. This makes a lot more 
> > difficult to confirm the doc movement was correct.
> You have to check it manually anyway, as the comment format is prefixing
> " * " to every line.
>
> Its not as if the changes would be THAT complicated, and the commit message
> is quite clear WHAT is changed.
You are breaking two conventional rules [1]:
  1. Patch should only be doing one logical thing
  2. Don't mix code clean-up and code changes

While there are a couple of cases we are tolerating it, I think doing 
code clean-up and code movement at the same time should always be avoided.
> I can do the split, of course, if you really need that (in this case I'd
> end up with 3 patches, one for the move, one for the "tag" entry, and one
> for the fix of the connection sequence).
That would be my preference because the simpler the patches are the 
easier they are to review.
Cheers,

[1] 
https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches#Break_down_your_patches_appropriately
--
Julien Grall