From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 911C7C4332F for ; Fri, 25 Nov 2022 21:35:07 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 5E82785221; Fri, 25 Nov 2022 22:35:05 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=denx.de Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1669412105; bh=hBXmrdwj4xMnNPAWJxJl7hbuckp1pybHE9lOWHnVCDk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=Jt3GxtqtB6euU0UjBGjJVfn/VLYleOm2Ly0b8dFcB0tF18BELaLzQxRQElxkiEnZu Xk4uzQox8nhFN15Ow71R6oEBxvwxLwqUlVvFY2irUX/qxctcPIv/z4l52G4s0jAH3Y c8PbCz1PktUx9KaE/lWrXj/SYgdKsG+RffF0Ke3vxEEevp1EMietTNxHll8gap/s12 8TbBAtwWo9zYlrC+46NpSlu7c3P3Gk4scvxC7BJe57DTDWN3JiDHRji+8vsrFr57eP NHm7KI0QRTQE8TNFOagAfVwViPYQpTp93jXiDT8r2RsQScTCkx5qCHjB89Xrjs/Tvi sUBppYuJ7IHxg== Received: from [127.0.0.1] (p578adb1c.dip0.t-ipconnect.de [87.138.219.28]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: marex@denx.de) by phobos.denx.de (Postfix) with ESMTPSA id E77AC8515F; Fri, 25 Nov 2022 22:35:01 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=denx.de; s=phobos-20191101; t=1669412102; bh=hBXmrdwj4xMnNPAWJxJl7hbuckp1pybHE9lOWHnVCDk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=UkvZy7AAqEt0OUHcrXmGp6sQ61lfMmDFx1+sytrTeCtqNJJWgnDFOpR+760kKRs8e 5UxK4qKCMSs7Pmvt8vcyg5l4cqpoU2Z9ur+fKMZw65WOqNeoWRuzG0wC+SH6f57+/z 7tmc6zOWf1slrup14EhQbI1XdDbua1V3FahxmATXCUBH9EckLnelfw+RepdiAJ4Q14 UeQrrPv2oaNODqw/iasY0q0jqT6JWlJQqCWaI3GnyDxrposT3zUnaz3IgjlmzYbf72 mKEswnCsCsqglW2gvSgAzhyp91ohDCX+wAYr/qn0fCiGcs5/90CTfHo1B4JNxgwXVs 0k1vDssA1pm1A== Message-ID: Date: Fri, 25 Nov 2022 22:35:01 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Subject: Re: [PATCH] cli_hush: fix 'exit' cmd that was not exiting scripts Content-Language: en-US To: Hector Palacios , u-boot@lists.denx.de Cc: festevam@gmail.com References: <20221118111937.24531-1-hector.palacios@digi.com> <3825c799-3c82-378a-5133-2f7c584e58d9@denx.de> <8299f0ef-cdd4-e7fe-56ea-0458e87d4de4@digi.com> From: Marek Vasut In-Reply-To: <8299f0ef-cdd4-e7fe-56ea-0458e87d4de4@digi.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On 11/21/22 17:32, Hector Palacios wrote: > On 11/21/22 09:55, Hector Palacios wrote: >> Hi Marek, >> >> On 11/19/22 15:12, Marek Vasut wrote: >>> On 11/18/22 12:19, Hector Palacios wrote: >>>> Commit 8c4e3b79bd0bb76eea16869e9666e19047c0d005 supposedly >>>> passed one-level up the argument passed to 'exit' but it also >>>> broke 'exit' purpose of stopping a script. >>>> >>>> In reality, even if 'do_exit()' is capable of returning any >>>> integer, the cli only admits '1' or '0' as return values. >>>> >>>> This commit respects the current implementation to allow 'exit' >>>> to at least return '1' for future processing, but returns >>>> when the command being run is 'exit'. >>>> >>>> Before this: >>>> >>>>       => setenv foo 'echo bar ; exit 3 ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>>       => setenv foo 'echo bar ; exit 1 ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>>       => setenv foo 'echo bar ; exit 0 ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>>       => setenv foo 'echo bar ; exit -1 ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>>       => setenv foo 'echo bar ; exit -2 ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>>       => setenv foo 'echo bar ; exit ; echo should not see this'; >>>> run foo; echo $? >>>>       bar >>>>       should not see this >>>>       0 >>>> >>>> After this: >>>> >>>>          => setenv foo 'echo bar ; exit 3 ; echo should not see >>>> this'; run foo; echo $? >>>>          bar >>>>          1 >>>>          => setenv foo 'echo bar ; exit 1 ; echo should not see >>>> this'; run foo; echo $? >>>>          bar >>>>          1 >>>>          => setenv foo 'echo bar ; exit 0 ; echo should not see >>>> this'; run foo; echo $? >>>>          bar >>>>          0 >>>>          => setenv foo 'echo bar ; exit -1 ; echo should not see >>>> this'; run foo; echo $? >>>>          bar >>>>          0 >>>>          => setenv foo 'echo bar ; exit -2 ; echo should not see >>>> this'; run foo; echo $? >>>>          bar >>>>          0 >>>>          => setenv foo 'echo bar ; exit ; echo should not see this'; >>>> run foo; echo $? >>>>          bar >>>>          0 >>>> >>>> Reported-by: Adrian Vovk >>>> Signed-off-by: Hector Palacios >>>> --- >>>>   common/cli_hush.c | 4 ++++ >>>>   1 file changed, 4 insertions(+) >>>> >>>> diff --git a/common/cli_hush.c b/common/cli_hush.c >>>> index 1467ff81b35b..9fe8b87e02d7 100644 >>>> --- a/common/cli_hush.c >>>> +++ b/common/cli_hush.c >>>> @@ -1902,6 +1902,10 @@ static int run_list_real(struct pipe *pi) >>>>                       last_return_code = -rcode - 2; >>>>                       return -2;      /* exit */ >>>>               } >>>> +             if (!strcmp(pi->progs->argv[0], "exit")) { >>>> +                     last_return_code = rcode; >>>> +                     return rcode;   /* exit */ >>>> +             } >>>>               last_return_code=(rcode == 0) ? 0 : 1; >>>>   #endif >>>>   #ifndef __U_BOOT__ >>> >>> Looking at the code just above this change 'if (rcode < -1) >>> last_return_code = -rcode - 2', that explains the odd 'return -r - 2' in >>> cmd/exit.c I think. >> >> That's what I thought, too. The cli captures a -2 as the number to >> exit a  script, and with -rcode -2 was exiting and returning a 0. >> Instead of capturing a magic number, I'm suggesting to capture 'exit' >> command. >> >> >>> I wonder, can we somehow fix the return code handling in cmd/exit.c >>> instead, so that it would cover both this behavior listed in this patch, >>> and 8c4e3b79bd0 ("cmd: exit: Fix return value") ? The cmd/exit.c seems >>> like the right place to fix it. >> >> I didn't revert or touched 8c4e3b79bd0 but if what you wanted to do >> with that commit is to return any positive integer to the upper >> layers, I must say that just doesn't work because the cli_hush only >> processes 1 (failure) or 0 (success), so there's no way for something >> such as 'exit 3' to produce a $? of 3. >> I think the 'exit' command should only be used with this old U-Boot >> standard of considering 1 a failure and 0 a success. >> >> I could remove the 'if (rcode < -1)  last_return_code = -rcode - 2', >> which doesn't add much value now, but other than that I'm unsure of >> what you have in mind as to fix cmd/exit.c. > > I just saw my patch causes a data abort on if conditionals, when > accessing argv[0]. > > Maybe we'd rather simply revert 8c4e3b79bd0 ("cmd: exit: Fix return > value") and let the exit command return 0 in all cases, as it is > documented, at least until we find a proper solution. We need to find a proper fix, no half-baked solutions. ( I need to think about the other email in this thread a bit more, I'll get back to you in a few days ).