Re: [Xen-devel] [PATCH] xen/tools/get-fields.sh: Use printf for POSIX compat

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.12.15 at 16:56, <alex_y_xu@xxxxxxxx> wrote:
> On Mon, 14 Dec 2015 02:12:16 -0700
> "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> 
>> >>> On 12.12.15 at 19:18, <alex_y_xu@xxxxxxxx> wrote:  
>> > xen/tools/get-fields.sh used echo -n which is not POSIX compatible
>> > and breaks with dash. Change it to use printf "%s" which is usable
>> > everywhere.  
>> 
>> Looks okay, but a couple of remarks:
>> - For the unaware as well as to know why to take care going forward,
>>   it would be nice if the commit message mentioned where actually you
>>   saw this to be an issue.
> 
> I already said that it doesn't work with dash. I can insert the word
> "building" if you want.

Oh, I'm sorry, I didn't take "dash" as a shell name (and have been
wondering what you mean with that sentence (taking the word as
just what ir normally means).

>> - There appears to be quite a bit of unnecessary quoting (namely on
>>   all the "%s" instances), making lines longer than they need to be
>>   (and hence possible harder to read).
> 
> Habit from C.
> 
>> In fact, since field names
>> can't contain %, \, or other characters with special meaning to
>> printf it looks as if the use of %s is superfluous altogether.
> 
> Again, habit from C, but here I think it is useful to use %s for
> several reasons.
> 
> 1. works exactly the same way as before
> 1a. no need to exhaustively check every case to see if % or \ ever gets
>     in
> 2. less likely to cause issues if someone copies and pastes the code or
>    the calls change later on

Well, in general I would agree, but in this particular case I wonder
whether the shorter resulting lines aren't outweighing the benefits
you name.

>> - Please Cc all relevant maintainers, not just Keir.
> 
> OK, I looked at MAINTAINERS but I only found "THE REST" and Keir was
> the only one who had touched get-fields.sh semi-recently.

But THE REST is it in this case.

Jan


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