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

[George Dunlap <dunlapg@umich.edu>]

On Tue, Mar 8, 2016 at 8:08 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.03.16 at 19:07, <dario.faggioli@citrix.com> wrote:
>> On Mon, 2016-03-07 at 04:46 -0700, Jan Beulich wrote:
>>> > > > On 04.03.16 at 19:45, <dario.faggioli@citrix.com> 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.

 -George

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