Re: [Xen-devel] [PATCH v2 3/3] xl: new "loglvl" command

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Mar 08, 2016 at 02:05:01PM +0000, George Dunlap wrote:
> On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
> >>>> On 07.03.16 at 19:07, <dario.faggioli@xxxxxxxxxx> wrote:
> >> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote:
> >>> > > > On 04.03.16 at 19:45, <dario.faggioli@xxxxxxxxxx> wrote:
> >>> > On Fri, 2016-03-04 at 09:48 -0700, Jan Beulich wrote:
> >>
> >>> > > --- a/tools/libxl/libxl.c
> >>> > > +++ b/tools/libxl/libxl.c
> >>> > > @@ -5958,6 +5958,26 @@ int libxl_send_debug_keys(libxl_ctx *ctx
> >>> > >      return 0;
> >>> > >  }
> >>> > >
> >>> > > +int libxl_log_level(libxl_ctx *ctx, bool set, bool guest,
> >>> > > +                    int *lower_thresh, int *upper_thresh)
> >>> > > +{
> >>> > > +    int ret;
> >>> > >
> >>> > As per libxl coding style, this wants to be 'r'.
> >>> This and everything else below look to be valid comments, but
> >>> it's rather frustrating that simply cloning an existing function (I
> >>> user the debug key ones as basis) doesn't give me valid code,
> >>> the more that I did scroll up and down a few pages to see
> >>> whether I just happened to pick a particularly bad example.
> >>>
> >> Hehe, but do you understand that, saying this, you're making it very
> >> likely that people will ask *you* to fix libxl_send_debug_keys() --and
> >> perhaps more tool side code? :-P :-P
> >
> > Except that it's not just that function - as said, I did scroll up and
> > down, without finding (style wise) better examples. And no, I'm
> > not going to put together patches to deal with style issues in the
> > tools.
> >
> >> No, jokes apart, I agree that inconsistency is a real bad thing... but
> >> it's an hard fight, and we do have examples spread all around the
> >> source code (both Xen and tools), AFAICT.
> >
> > Right, and asking people myself to not follow bad examples when
> > adding new code, I did take all of your input to adjust the patch.
> > Just that in this case the set of bad examples is so large that in a
> > similar case in the hypervisor I probably wouldn't have dared to
> > ask for a style correction.
> 
> Well the approach of the libxl maintainers seems to have be, "Just
> make sure the new code adheres to the new style, and eventyally
> everything will be up-to-date".  A few releases ago I submitted a
> patch where I added a new clause in the middle of a series of other
> very similar clauses, and I was asked to make the new clause follow
> the new style, but to leave the other clauses right next to it in the
> old style (to avoid unnecessary code churn, since it was during the
> development freeze).
> 
> Given that the "new" style has been around for a while now, it
> probably would be good to set aside some time at the beginning of the
> next development cycle to fix things up -- it is incredibly
> frustrating to carefully try to copy the surrounding style, only to be
> told to revise it.
> 

I did fix a few hundred instances of inconsistency at the beginning of
4.7 cycle. Spatch is helpful, but it is far from perfect.

What I'm afraid of is that fixing them would introduce too much noise
that renders file line annotation useless.

Wei.

>  -George

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