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

[Alex Xu <alex_y_xu@xxxxxxxx>]

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.

> - 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

> - 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.

> 
> Jan
> 

Thank you for the feedback. I will rewrite the commit message and send
again to everyone.

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