Re: [Xen-devel] [PATCH 0/1] xl.cfg man page cleanup and fixes

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

Armando Vega writes ("[PATCH 0/1] xl.cfg man page cleanup and fixes"):
> so I've made a new round of cleaning and fixing. There was quite some work to
> be done with this one. And there are a few issues that are left still, but
> at least not with the general correctness and style of the manual. More info
> below.

Thanks for your excellent work.

Thanks also for your clarity in this message:

> I've had to rework the NUMA node examples as it had what I would call a
> counting error and in the end presented incorrect information. It would be
> great if someone could check me up on that once more. Also, there is no clear
> explanation whether a person can use ^nodes:1 and nodes:^1 interchangably and
> to be honest I wasn't sure myself. Haven't had the time to give this proper
> testing.

I think this means we should get a review from Dario or George (added
to the To: field).

> Also there is an issue with at least one of the HVM-only options
> that can actually be used with PV guests as well. I know because
> we've been using CPU masking / feature leveling for our PV guests on
> Xen 4.6., and if it went from being HVM-only to also on PV I
> couldn't say when that actually happened.  It is quite possible that
> there are more such options which aren't exclusive to one type
> anymore.
> 
> Anyway, that is something to be discussed and fixed in another iteration.

Fair enough.


I have a couple of suggestions for improvement to your approach:

Some of the information you have put in this 0/1 cover letter could
usefully be in the commit message for the patch, instead.  In
particular, the commit message ought to explain that you are making a
semantic change and why.  If you need to respin this patch, please do
that.


The other point is: did you see my comment about semantic linefeeds ?
I wrote:

   Rather, if a line gets too long, break it at a phrase or sentence
   boundary.  That means that future diffs are much more readable.

   http://rhodesmill.org/brandon/2012/one-sentence-per-line/

Ie, instead of this

>  Specifies the disks (both emulated disks and Xen virtual block
>  devices) which are to be provided to the guest, and what objects on
> -the host they should map to.  See L<xl-disk-configuration(5)>.
> +the host they should map to.  See L<xl-disk-configuration(5)> for more
> +details.

consider this:

>  Specifies the disks (both emulated disks and Xen virtual block
>  devices) which are to be provided to the guest, and what objects on
> -the host they should map to.  See L<xl-disk-configuration(5)>.
> +the host they should map to.
> +See L<xl-disk-configuration(5)> for more details.
 
You don't usually see the benefit of this approach in the patch where
you start doing it, but it makes future edits clearer and easier.

I don't know whether you haven't been doing that in your changes
because you didn't agree with it, or didn't want to do that as well as
your other changes (in which case fine), or because you missed it or
didn't understand it (hence the longer explanation here).


Regards,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel