[2/4] scripts/config: use sed's POSIX interface
diff mbox

Message ID 83e8b90e1d2cc5ff5d2443f2486c2d786a4997ce.1376600922.git.yann.morin.1998@free.fr
State New, archived
Headers show

Commit Message

Yann E. MORIN Aug. 15, 2013, 9:17 p.m. UTC
From: Clement Chauplannaz <chauplac@gmail.com>

Script `config' relies on extensions of `GNU sed', and is thus not
working on all Unixes:
  - in-place edition of files (-i), which can be replaced with
    a temporary file;
  - extended-regexps (-r), which can be split into basic regexps;
  - single-line calls to `a' command, while some implementations
    require a leading newline before the parameter.

Rewrite calls to `sed' to comply with POSIX interface, and move them
to helper functions.

Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
---
 scripts/config | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

Comments

Linus Walleij Sept. 12, 2013, 4:40 p.m. UTC | #1
On Thu, Aug 15, 2013 at 11:17 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:

> From: Clement Chauplannaz <chauplac@gmail.com>
>
> Script `config' relies on extensions of `GNU sed', and is thus not
> working on all Unixes:
>   - in-place edition of files (-i), which can be replaced with
>     a temporary file;
>   - extended-regexps (-r), which can be split into basic regexps;
>   - single-line calls to `a' command, while some implementations
>     require a leading newline before the parameter.
>
> Rewrite calls to `sed' to comply with POSIX interface, and move them
> to helper functions.
>
> Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>

This patch totally breaks my usage of the --set-str
argument to "config".

Reverting this patch solves the problem.

Reproduce:
make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
KBUILD_OUTPUT=build-nomadik nhk8815_defconfig
scripts/config --file build-nomadik/.config --set-str CMDLINE
"root=/dev/ram0 console=ttyAMA1,115200n8 earlyprintk"
sed: -e uttryck #1, tecken 44: flaggan okänd för "s"
sed: -e uttryck #1, tecken 54: flaggan okänd för "s"

Swedish messages meaning "unknown flag for "s""

After reverting the patch these messages no longer appear.

At failure my config file is scratched :-O

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Clement Chauplannaz Sept. 13, 2013, 8:38 a.m. UTC | #2
2013/9/12 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Aug 15, 2013 at 11:17 PM, Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
>> From: Clement Chauplannaz <chauplac@gmail.com>
>>
>> Script `config' relies on extensions of `GNU sed', and is thus not
>> working on all Unixes:
>>   - in-place edition of files (-i), which can be replaced with
>>     a temporary file;
>>   - extended-regexps (-r), which can be split into basic regexps;
>>   - single-line calls to `a' command, while some implementations
>>     require a leading newline before the parameter.
>>
>> Rewrite calls to `sed' to comply with POSIX interface, and move them
>> to helper functions.
>>
>> Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
>> Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
>> Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr>
>
> This patch totally breaks my usage of the --set-str
> argument to "config".
>
> Reverting this patch solves the problem.
>
> Reproduce:
> make ARCH=arm CROSS_COMPILE=arm-linux-gnueabi-
> KBUILD_OUTPUT=build-nomadik nhk8815_defconfig
> scripts/config --file build-nomadik/.config --set-str CMDLINE
> "root=/dev/ram0 console=ttyAMA1,115200n8 earlyprintk"
> sed: -e uttryck #1, tecken 44: flaggan okänd för "s"
> sed: -e uttryck #1, tecken 54: flaggan okänd för "s"
>
> Swedish messages meaning "unknown flag for "s""
>
> After reverting the patch these messages no longer appear.
>
> At failure my config file is scratched :-O
>
> Yours,
> Linus Walleij

Hello Linus,

Thank you for this report. I was able to reproduce this bug and fix it.

My previous commit changed the separator between sed's substitute
command and its parameters, from ':' to '/'. The latter conflicted
with the slashes found in the value of variable CMDLINE, as provided
in your email.

I'm sending a patch right now to revert to previous behavior.

Best regards,
Clement Chauplannaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Sept. 13, 2013, 9:32 a.m. UTC | #3
On Fri, Sep 13, 2013 at 10:38 AM, Clément Chauplannaz
<chauplac@gmail.com> wrote:

> Thank you for this report. I was able to reproduce this bug and fix it.

Thanks! Tested and works fine.

> My previous commit changed the separator between sed's substitute
> command and its parameters, from ':' to '/'. The latter conflicted
> with the slashes found in the value of variable CMDLINE, as provided
> in your email.

Hm it could actually be useful to be able to have colons in a CMDLINE,
I wonder if we can think about some better separator ... oh well that
is another issue, all old scripts work now anyway.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Clement Chauplannaz Sept. 13, 2013, 9:54 a.m. UTC | #4
On Sep 13, 2013, at 11:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Fri, Sep 13, 2013 at 10:38 AM, Clément Chauplannaz
> <chauplac@gmail.com> wrote:
> 
>> Thank you for this report. I was able to reproduce this bug and fix it.
> 
> Thanks! Tested and works fine.
Glad to read the patch solves your issue. Thanks for the quick feedback!
> 
>> My previous commit changed the separator between sed's substitute
>> command and its parameters, from ':' to '/'. The latter conflicted
>> with the slashes found in the value of variable CMDLINE, as provided
>> in your email.
> 
> Hm it could actually be useful to be able to have colons in a CMDLINE,
> I wonder if we can think about some better separator ... oh well that
> is another issue, all old scripts work now anyway.
Indeed config script may not work with all possible string values. My first concern for now was to fallback to previous interface. We may look into hardening the script later on.

Best regards,
Clement Chauplannaz--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Marek Sept. 13, 2013, 11 a.m. UTC | #5
Dne 13.9.2013 11:54, Clément Chauplannaz napsal(a):
> On Sep 13, 2013, at 11:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
>> On Fri, Sep 13, 2013 at 10:38 AM, Clément Chauplannaz
>> <chauplac@gmail.com> wrote:
>>
>>> Thank you for this report. I was able to reproduce this bug and fix it.
>>
>> Thanks! Tested and works fine.
> Glad to read the patch solves your issue. Thanks for the quick feedback!
>>
>>> My previous commit changed the separator between sed's substitute
>>> command and its parameters, from ':' to '/'. The latter conflicted
>>> with the slashes found in the value of variable CMDLINE, as provided
>>> in your email.
>>
>> Hm it could actually be useful to be able to have colons in a CMDLINE,
>> I wonder if we can think about some better separator ... oh well that
>> is another issue, all old scripts work now anyway.
> Indeed config script may not work with all possible string values.
> My
> first concern for now was to fallback to previous interface. We may look
> into hardening the script later on.

Right. I will merge the patch because it reverts a regression. But feel
free to submit another patch that escapes the colons in $after. The
script already uses #!/bin/bash, so a "${after//:/\:}" should work.

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/scripts/config b/scripts/config
index 567120a..2283be2 100755
--- a/scripts/config
+++ b/scripts/config
@@ -62,15 +62,52 @@  checkarg() {
 	fi
 }
 
+txt_append() {
+	local anchor="$1"
+	local insert="$2"
+	local infile="$3"
+	local tmpfile="$infile.swp"
+
+	# sed append cmd: 'a\' + newline + text + newline
+	cmd="$(printf "a\\%b$insert" "\n")"
+
+	sed -e "/$anchor/$cmd" "$infile" >"$tmpfile"
+	# replace original file with the edited one
+	mv "$tmpfile" "$infile"
+}
+
+txt_subst() {
+	local before="$1"
+	local after="$2"
+	local infile="$3"
+	local tmpfile="$infile.swp"
+
+	sed -e "s/$before/$after/" "$infile" >"$tmpfile"
+	# replace original file with the edited one
+	mv "$tmpfile" "$infile"
+}
+
+txt_delete() {
+	local text="$1"
+	local infile="$2"
+	local tmpfile="$infile.swp"
+
+	sed -e "/$text/d" "$infile" >"$tmpfile"
+	# replace original file with the edited one
+	mv "$tmpfile" "$infile"
+}
+
 set_var() {
 	local name=$1 new=$2 before=$3
 
 	name_re="^($name=|# $name is not set)"
 	before_re="^($before=|# $before is not set)"
 	if test -n "$before" && grep -Eq "$before_re" "$FN"; then
-		sed -ri "/$before_re/a $new" "$FN"
+		txt_append "^$before=" "$new" "$FN"
+		txt_append "^# $before is not set" "$new" "$FN"
 	elif grep -Eq "$name_re" "$FN"; then
-		sed -ri "s:$name_re.*:$new:" "$FN"
+		txt_subst "^$name=.*" "$new" "$FN"
+		txt_subst "^# $name is not set" "$new" "$FN"
 	else
 		echo "$new" >>"$FN"
 	fi
@@ -79,7 +116,8 @@  set_var() {
 undef_var() {
 	local name=$1
 
-	sed -ri "/^($name=|# $name is not set)/d" "$FN"
+	txt_delete "^$name=" "$FN"
+	txt_delete "^# $name is not set" "$FN"
 }
 
 if [ "$1" = "--file" ]; then