> -----Original Message----- > From: Stephen Warren [mailto:swar...@wwwdotorg.org] > Sent: Saturday, November 24, 2012 7:01 PM > To: Piotr Wilczek > Cc: u-boot@lists.denx.de; 'Minkyu Kang'; 'Kyungmin Park'; 'Chang Hyun > Park'; Lukasz Majewski; 'Stephen Warren'; 'Tom Rini'; 'Rob Herring' > Subject: Re: [PATCH v4 6/7] gpt: Support for new "gpt" command > > On 11/21/2012 04:22 AM, Piotr Wilczek wrote: > > Dear Stephen, > > > >> Stephen Warren wrote at Monday, November 19, 2012 10:35 PM: > >> On 11/09/2012 03:48 AM, Piotr Wilczek wrote: > >>> New command - "gpt" is supported. It restores the GPT partition > table. > >>> It looks into the "partitions" environment variable for partitions > definition. > >>> It can be enabled at target configuration file with CONFIG_CMD_GPT. > >>> Simple UUID generator has been implemented. It uses the the > >>> gd->start_addr_sp for entrophy pool. Moreover the pool address is > used as crc32 seed. > >> > >>> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c > >> > >>> +U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt, > >>> + "GUID Partition Table", > >>> + "<interface> <dev> <partions list>\n" > >>> + " partions list is in format: name=..,size=..,uuid=..;...\n" > >>> + " and can be passed as env or string ex.:\n" > >>> + " gpt mmc 0 partitions\n" > >> > >> I don't think that form makes sense. The user should just pass > >> "${partitions}" instead. The command can't know for certain whether > >> the user actually intended to pass the text "partitions" and made a > >> mistake, or whether they passed an environment variable. If you > >> really want to be able to pass an environment variable name, an > >> explicit command-line option such as: > >> > >> gpt mmc 0 name=... # definition on cmd-line > >> gpt mmc 0 --from-environment partitions # definition in environment > >> > >> seems best. > > > > The intention was that the command automatically figures out whether > > user passed environmental variable or directly partitions as text. > > Then the command splits this string, checks if it is a valid > > partitions list and if so the table is written to mmc. That is how > the code is supposed to work. > > > > The question here is, if it should work like that. If it is desired > > that user explicitly states that the partitions list is passed from > > environmental, then I should change the code. > > I personally prefer things to be explicit; that way, there can't be any > corner-case that isn't covered by the automatic mode. > Ok. The partitions list will be passed in two methods: gpt create mmc 0 ${partitions_env_name} - from environmental or gpt create mmc 0 "name=..,size=..,uuid=..;..." - form text
In both cases any partition parameter can also be passes as env: gpt create mmc 0 "name=..,size=${part1_size},uuid=..;..." > >>> +/** > >>> + * extract_env(): Convert string from '&{env_name}' to 'env_name' > >> > >> s/&/$/ > >> > >> It's doing more than that; it locates that syntax within an > arbitrary > >> string and ignores anything before "${" or after "}". Is that > >> intentional? > > > > Yes, it was. The u-boot's shell expands to one only, so it allow to > > pass any partition parameter as env when the partitions list itself > is passed as env. > > OK. The issue here is that the comment doesn't exactly describe what > the code is doing. > > Also, what if the user wrote "foo${var}bar"; I can't recall if the code > handles that correct; is the result of that just "${var}", or do "foo" > and "bar" actually make it into the result string? The 'bar' will be dropped, but to drop 'foo' a small modification is needed. > > >>> +static int extract_env(char *p) > >> > >>> + p1 = strstr(p, "${"); > >>> + p2 = strstr(p, "}"); > >>> + > >>> + if (p1 && p2) { > >>> + *p2 = '\0'; > >>> + memmove(p, p+2, p2-p1-1); > >> > >> s/-1/-2/ I think, since the length of "${" is 2 not 1. > >> > > p2-p1-1 gives length of the env name + trailing zero. > > p2-p1-2 would give only the env's length and the trailing zero > > wouldn't be moved. > > Ah right. I tend to write things like that as: > > p2-p1-2+1 /* strlen("${")==2, length '\0'==1 > > to make it obvious what's going on > > >>> + tok = strsep(&p, ";"); > >>> + if (tok == NULL) > >>> + break; > >>> + > >>> + if (extract_val(&tok, name, i, "name")) { > >>> + ret = -1; > >>> + goto err; > >>> + } > >>> + > >>> + if (extract_val(&tok, ps, i, "size")) { > >>> + ret = -1; > >>> + free(name[i]); > >>> + goto err; > >>> + } > >> > >> I think that requires the parameters to be passed in order > >> name=foo,size=5,uuid=xxx. That seems inflexible. The syntax may as > >> well just be value,value,value rather than > >> key=value,key=value,key=value in that case (although the keys are > >> useful in order to understand the data, so I'd prefer parsing > flexibility rather than removing key=). > >> > > I would say that the "key=value" is flexible. The 'extract_env' > > function tells you if the requested key was provided or not. Also, > the > > order of keys is not important. > > The order of the keys shouldn't be important, but doesn't the code > above expect to find key "name" first, then key "size", etc., in tha > specific order, as it walks through the string? The key name is passed to 'extract_val' and the function should search that key no matter what order the keys appear in searched string. I check it again. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot