Patchwork [2/6] layout: Add -i <region>[:<file>] support.

2013-10-08 00:34:09

Am 24.09.2013 01:14 schrieb Stefan Tauner:
> Add an optional sub-parameter to the -i parameter to allow building the
> image to be written from multiple files. This will also allow regions to
> be read from flash and written to separate image files in a later patch.
> Existing function read_buf_from_file() is refined and reused to read the file.
>
> based on chromiumos'
> d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8
> Signed-off-by: David Hendricks <dhendrix@chromium.org>
> Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>

Counter-proposal, slightly edited patch, comments below are addressed.

> diff --git a/cli_classic.c b/cli_classic.c
> index a0c2d64..ed6d6aa 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -40,8 +40,8 @@ static void cli_classic_usage(const char *name)
>  #if CONFIG_PRINT_WIKI == 1
>  	       "-z|"
>  #endif
> -	       "-p <programmername>[:<parameters>] [-c <chipname>]\n"
> -	       "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...] [-n] [-f]]\n"
> +	       "-p <programmername>[:<parameter>[,<parameter>]...] [-c <chipname>]\n"
> +	       "[-E|(-r|-w|-v) <imagefile>] [-l <layoutfile> [-i <region>[:<regionfile>]...] [-n] [-f]]\n"

80 col violation. Unclosed [.

>  	       "[-V[V[V]]] [-o <logfile>]\n\n", name);
>  
>  	printf(" -h | --help                        print this help text\n"
> diff --git a/layout.c b/layout.c
> index 08cc776..7e2e904 100644
> --- a/layout.c
> +++ b/layout.c
> @@ -165,27 +161,61 @@ int process_include_args(void)
>  	if (num_include_args == 0)
>  		return 0;
>  
> -	/* User has specified an area, but no layout file is loaded. */
> +	/* User has specified an include argument, but no layout file is loaded. */
>  	if (num_rom_entries == 0) {
> -		msg_gerr("Region requested (with -i \"%s\"), "
> -			 "but no layout data is available.\n",
> +		msg_gerr("Region requested (with -i/--include \"%s\"),\n"
> +			 "but no layout data is available. To include one use the -l/--layout syntax).\n",

"To include one [layout data] ..." sounds odd. I rewrote it slightly.

>  			 include_args[0]);
>  		return 1;
>  	}
>  
>  	for (i = 0; i < num_include_args; i++) {
> -		if (find_romentry(include_args[i]) < 0) {
> -			msg_gerr("Invalid region specified: \"%s\".\n",
> -				 include_args[i]);
> +		char *file;
> +		char *name = include_args[i];
> +		int ret = unquote_string(&name, &file, ":"); /* -i <region>[:<file>] */

I killed the unquote_string requirement here. Whitespace and colons have
been outlawed in region names.

> +		if (ret != 0) {
> +			msg_gerr("Invalid include argument specified: \"%s\".\n", name);
>  			return 1;
>  		}
> +		int idx = find_romentry(name);
> +		if (idx < 0) {
> +			msg_gerr("Invalid region name specified: \"%s\".\n", name);
> +			return 1;
> +		}
> +		rom_entries[idx].included = 1;
>  		found++;
> +
> +		if (file[0] != '\0') {
> +			/* The remaining characters are interpreted as possible quoted filename.  */
> +			ret = unquote_string(&file, NULL, NULL);

A file name on the command line should never require quote parsing in
the application. Same for any other flashrom UI.

> +			if (ret != 0) {
> +				msg_gerr("Invalid region file name specified: \"%s\".\n", file);
> +				return 1;
> +			}
> +#ifdef __LIBPAYLOAD__
> +			msg_gerr("Error: No file I/O support in libpayload\n");
> +			return 1;
> +#else
> +			file = strdup(file);
> +			if (file == NULL) {
> +				msg_gerr("Out of memory!\n");
> +				return 1;
> +			}
> +			rom_entries[idx].file = file;
> +#endif
> +		}
>  	}
>  
> -	msg_ginfo("Using region%s: \"%s\"", num_include_args > 1 ? "s" : "",
> -		  include_args[0]);
> -	for (i = 1; i < num_include_args; i++)
> -		msg_ginfo(", \"%s\"", include_args[i]);
> +	msg_ginfo("Using region%s: ", num_rom_entries > 1 ? "s" : "");

Should be found, not num_rom_entries.

> +	bool first = true;
> +	for (i = 0; i < num_rom_entries; i++)
> +		if (rom_entries[i].included) {
> +			if (first)
> +				first = false;
> +			else
> +				msg_ginfo(", ");
> +			msg_ginfo("\"%s\"", rom_entries[i].name);
> +	}
>  	msg_ginfo(".\n");
>  	return 0;
>  }

There was another bug: register_include_arg() tried to detect duplicate
include region arguments. It could only detect identical include
strings, but not stuff like this:
-i region1 -i region1:region1file.bin
The duplicate --include detection has been moved to process_include_args().
Patch 1/6 is no longer a requirement for 2/6 and can thus be committed
after 2/6.

TODO: Move the relevant man page chunk from 3/6 to 2/6. Will do so in a
followup mail.

Add an optional sub-parameter to the -i parameter to allow building the
image to be written from multiple files. This will also allow regions to
be read from flash and written to separate image files in a later patch.
Existing function read_buf_from_file() is refined and reused to read the
file.
Document criteria for valid region names.
Based on chromiumos' d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8

Signed-off-by: David Hendricks <dhendrix@chromium.org>
Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>
Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>

Submitter	Carl-Daniel Hailfinger
Date	2013-10-08 00:34:09
Message ID	<52535301.2080300@gmx.net>
Download	mbox \| patch
Permalink	/patch/4075/
State	New
Headers	show Return-path: <flashrom-bounces@flashrom.org> Envelope-to: patchwork@coreboot.org Received: from localhost ([127.0.0.1] helo=ra.coresystems.de) by ra.coresystems.de with esmtp (Exim 4.77) (envelope-from <flashrom-bounces@flashrom.org>) id 1VTLFv-0006FL-CV; Tue, 08 Oct 2013 02:34:31 +0200 Received: from mout.gmx.net ([212.227.17.21]) by ra.coresystems.de with esmtps (TLSv1:AES128-SHA:128) (Exim 4.77) (envelope-from <c-d.hailfinger.devel.2006@gmx.net>) id 1VTLFl-0006Ch-Mn for flashrom@flashrom.org; Tue, 08 Oct 2013 02:34:30 +0200 Received: from [93.129.128.181] ([93.129.128.181]) by mail.gmx.com (mrgmx002) with ESMTPSA (Nemesis) id 0LlESk-1W3EIf41qA-00b0xX for <flashrom@flashrom.org>; Tue, 08 Oct 2013 02:34:12 +0200 Message-ID: <52535301.2080300@gmx.net> Date: Tue, 08 Oct 2013 02:34:09 +0200 From: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> User-Agent: Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0 SeaMonkey/2.17 MIME-Version: 1.0 To: Stefan Tauner <stefan.tauner@student.tuwien.ac.at>, flashrom@flashrom.org References: <1379546978-24834-1-git-send-email-stefan.tauner@student.tuwien.ac.at> <1379978046-32403-1-git-send-email-stefan.tauner@student.tuwien.ac.at> <1379978046-32403-3-git-send-email-stefan.tauner@student.tuwien.ac.at> In-Reply-To: <1379978046-32403-3-git-send-email-stefan.tauner@student.tuwien.ac.at> X-Enigmail-Version: 1.5.2 X-Provags-ID: V03:K0:2gDbPoigjjfQKsMSC1o7tuSmqdDTMc0QuhkWbbF45+B+2xLgaMF 11FWSLWjSNYbpnMcf3LklCU4/WmJRFvUkWJIh/9jU4hRDttImf+J7x2k2fZp90efJjYYDTM jDxrorqEfs7b5rbtkKpZkq7f3UbGqPNaOAXCQClTMka+E6LIPd1cxQIky8MjfZpyG4U8PYh g/+FuLnqMU2gzjNo3e7MA== X-Spam-Score: -0.5 (/) Cc: David Hendricks <dhendrix@chromium.org> Subject: Re: [flashrom] [PATCH 2/6] layout: Add -i <region>[:<file>] support. X-BeenThere: flashrom@flashrom.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: flashrom discussion and development mailing list <flashrom.flashrom.org> List-Unsubscribe: <http://www.flashrom.org/mailman/options/flashrom>, <mailto:flashrom-request@flashrom.org?subject=unsubscribe> List-Archive: <http://www.flashrom.org/pipermail/flashrom> List-Post: <mailto:flashrom@flashrom.org> List-Help: <mailto:flashrom-request@flashrom.org?subject=help> List-Subscribe: <http://www.flashrom.org/mailman/listinfo/flashrom>, <mailto:flashrom-request@flashrom.org?subject=subscribe> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: flashrom-bounces@flashrom.org Sender: flashrom-bounces@flashrom.org

Patchwork [2/6] layout: Add -i <region>[:<file>] support.

Comments

Patch