Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 15 Jun 2020 15:14:42 -0400
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Masami Hiramatsu <mhiramat@kernel.org>
Cc:     stable@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to
 show the bootconfig
Message-ID: <20200615151442.028c2876@oasis.local.home>
In-Reply-To: <159197541534.80267.9851345208191438725.stgit@devnote2>
References: <159197538852.80267.10091816844311950396.stgit@devnote2>
        <159197541534.80267.9851345208191438725.stgit@devnote2>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, 13 Jun 2020 00:23:35 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Fix bootconfig to return 0 if succeeded to show the bootconfig
> in initrd. Without this fix, "bootconfig INITRD" command
> returns !0 even if the command succeeded to show the bootconfig.
> 
> Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
> Cc: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  tools/bootconfig/main.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 21896a6675fd..ff2cc9520e10 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -209,8 +209,10 @@ int show_xbc(const char *path)
>  	ret = load_xbc_from_initrd(fd, &buf);
>  	if (ret < 0)
>  		pr_err("Failed to load a boot config from initrd: %d\n", ret);
> -	else
> +	else {
>  		xbc_show_compact_tree();
> +		ret = 0;
> +	}

Usually for the above, I think goto is cleaner. As it is strange to
have a successful case as the "else" condition. Not to mention, if you
add brackets for one side of the else, it is usually recommended to add
them for the other side.

But in this case I think it would read better to have:


	if (ret < 0) {
		pr_err(...);
		goto out;
	}
	xbc_show_compact_tree();
	ret = 0;
out:
	
>  
>  	close(fd);
>  	free(buf);

-- Steve