| Message ID | 722df36b26b2bb5657a113a7aabe72d95e475ca4.1393600504.git.p.marczak@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote: > Changes: > - randomly generate each partition uuid if undefined > - print info about generated uuid > - save environment on gpt write success > - update doc/README.gpt > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c > static char extract_env(const char *str, char **env) > { > + int ret = -1; Why does the function return char not int? At least the type of ret should match the return type of the function. There's no need to introduce a ret variable anyway; just don't delete the return statements that are already in the function. > @@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env) > memset(s + strlen(s) - 1, '\0', 1); > memmove(s, s + 2, strlen(s) - 1); > e = getenv(s); > - free(s); > if (e == NULL) { > - printf("Environmental '%s' not set\n", str); > - return -1; /* env not set */ > + printf("%s unset. ", str); > + e = get_uuid_str(); > + if (e) { > + printf("Setting to: %s\n", e); > + setenv(s, e); Why should the environment variable be set? I rather dislike commands that randomly set environment variables as an implicit side-effect. It'd be far better if this function simply wasn't modified, but rather the user was provided with a function to explicitly set an environment variable to a randomly generated GPT. That way the user/script would be in control. Something like: $ gen_random_uuid env_var_name > @@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > return CMD_RET_FAILURE; > } > > - if (gpt_default(blk_dev_desc, argv[4])) > + puts("Writing GPT: "); > + > + ret = gpt_default(blk_dev_desc, argv[4]); > + if (!ret) { > + puts("success!\n"); > + saveenv(); Uggh. Definitely don't save the environment behind the user's back. There is no reason to believe that's safe. What if the user had added some temporary changes to their environment that they didn't want saved? This kind of logic belongs in scripts, not code.
Hello again, On 02/28/2014 06:03 PM, Stephen Warren wrote: > On 02/28/2014 08:18 AM, Przemyslaw Marczak wrote: >> Changes: >> - randomly generate each partition uuid if undefined >> - print info about generated uuid >> - save environment on gpt write success >> - update doc/README.gpt > >> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c > >> static char extract_env(const char *str, char **env) >> { >> + int ret = -1; > > Why does the function return char not int? At least the type of ret > should match the return type of the function. > Right notice, this should be fixed. > There's no need to introduce a ret variable anyway; just don't delete > the return statements that are already in the function. > But I need to move "free(s)" so I can't leave current return statements. Other way I need to put "free(s)" in few places. >> @@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env) >> memset(s + strlen(s) - 1, '\0', 1); >> memmove(s, s + 2, strlen(s) - 1); >> e = getenv(s); >> - free(s); >> if (e == NULL) { >> - printf("Environmental '%s' not set\n", str); >> - return -1; /* env not set */ >> + printf("%s unset. ", str); >> + e = get_uuid_str(); >> + if (e) { >> + printf("Setting to: %s\n", e); >> + setenv(s, e); And here I forget about free(e)... > > Why should the environment variable be set? I rather dislike commands > that randomly set environment variables as an implicit side-effect. > Actually automatically generated uuids was the main purpose of this patches. Setting each env variable in this place was the most easy way to make this without a lot of duplicated code. Why do you treat it like a side-effect? If user wants have own generated uuids - then he can manually set env variables like "uuid_gpt_disk". This actually is not changed - when uuid env is set then it will be used like in current version of code. When user can't generate uuids or just wants to have it automatically generated then my code do this job. > It'd be far better if this function simply wasn't modified, but rather > the user was provided with a function to explicitly set an environment > variable to a randomly generated GPT. That way the user/script would be > in control. Something like: > > $ gen_random_uuid env_var_name > I understand that this is very important code, but setting each val manually with random uuid actually will not change anything - user still needs to do something. The other way is to provide a function for parse e.g $partitions but then it will be a code duplication. The main job is done by set_gpt_info() so this is why I modified this code. >> @@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >> return CMD_RET_FAILURE; >> } >> >> - if (gpt_default(blk_dev_desc, argv[4])) >> + puts("Writing GPT: "); >> + >> + ret = gpt_default(blk_dev_desc, argv[4]); >> + if (!ret) { >> + puts("success!\n"); >> + saveenv(); > > Uggh. Definitely don't save the environment behind the user's back. > There is no reason to believe that's safe. What if the user had added > some temporary changes to their environment that they didn't want saved? > This kind of logic belongs in scripts, not code. > > The one and only reason for put saveenv() here was that if uuids are randomly generated or even just are in environment then I can be sure that next gpt write (e.g. in case of overwrite sector 0 by mistake) is using the same uuids values. Maybe saveenv() in this place is not the best idea but in other side user actually uses this command just once. Thank you
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: [snip] > Actually automatically generated uuids was the main purpose of this > patches. Setting each env variable in this place was the most easy way > to make this without a lot of duplicated code. > > Why do you treat it like a side-effect? > If user wants have own generated uuids - then he can manually set env > variables like "uuid_gpt_disk". > This actually is not changed - when uuid env is set then it will be used > like in current version of code. > When user can't generate uuids or just wants to have it automatically > generated then my code do this job. Having been using this code again myself recently, at the high level, this is useful. Having to copy/paste in N UUIDs gets silly. But I also wonder.. [snip] > The one and only reason for put saveenv() here was that if uuids are > randomly generated or even just are in environment then I can be sure > that next gpt write (e.g. in case of overwrite sector 0 by mistake) is > using the same uuids values. Is this really an important use case to cover? The way I see things, would it be possible (and not a pain) to make the UUID part of the partition string passed to 'gpt write' optional. If not passed, generate the UUIDs needed. What was used would be seen in 'part list' and so forth. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTFI32AAoJENk4IS6UOR1WAyQP/2bfClvRaWtLzUDggRUnNr99 RKSgB1m/nl09ZwclB2Gn1FZL2IR/M0u+ia89FE9leSN4eoOEswN2rrUiExVv7QpJ d+oCH/H3oNDAyb9L4HXXYHeRGCVHK7KE/KP5Ngb7KfTGZhj5kHdkx9iQM7dO9OtX DmvW5+JoRXgXPkpmvy5s20IzRUbBEqGC6Z2h8Y/VKHTDnrUmYvFb6XPgjwwrYVB6 +Hx/7K2R1uXQTuoHM7FqVAqKx7GGiUmcpLG90iHeYcX4fib/+RwC7hcre0rRj71v NyAnO4t0nXKefLXYe5iGold7cNx9xUKO3s4u2EkC9lNGRkN6RcVuaxzEKGD4/QTX fHA2q6FwSVLv4lXBT6UWxXiky2C9TMY/DUNM0EWe2KSx2V2glXPKiBX7gRk0Rq0d EWq1G5oQS0qiZgb+vUQ4Acf4/HDjhAl1l18Hx6w+26LNvz8Fi+o7Om4kkBdI5sos GX2auS1RxQc3qQkrAjxFx1xpDr9iMikV6nNqYDcVU894PeCgzmUWaWG/IrBFixO6 2XjF1sMth8LrVqUHirh3Y7lDU+FFPP+mMb6eaY3oajtVkg3u6cpQ0eJ0A612CsXG oaTPxWKKTsWlBxaVIgzu2OXeYg5BJF4OTKxjNiV2wptheEVc4RVLDiQ1yf74YEHy Hve7lpxa9i4Oo9Vf2Pd7 =5FKf -----END PGP SIGNATURE-----
Hello Tom, On 03/03/2014 03:13 PM, Tom Rini wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: > > [snip] >> Actually automatically generated uuids was the main purpose of this >> patches. Setting each env variable in this place was the most easy way >> to make this without a lot of duplicated code. >> >> Why do you treat it like a side-effect? >> If user wants have own generated uuids - then he can manually set env >> variables like "uuid_gpt_disk". >> This actually is not changed - when uuid env is set then it will be used >> like in current version of code. >> When user can't generate uuids or just wants to have it automatically >> generated then my code do this job. > > Having been using this code again myself recently, at the high level, > this is useful. Having to copy/paste in N UUIDs gets silly. But I also > wonder.. > > [snip] >> The one and only reason for put saveenv() here was that if uuids are >> randomly generated or even just are in environment then I can be sure >> that next gpt write (e.g. in case of overwrite sector 0 by mistake) is >> using the same uuids values. > > Is this really an important use case to cover? > It can be important if somebody uses UUIDS to boot kernel. In kernel documentation you can find a notice about kernel function name_to_dev_t() - so by command line you can pass uuid for root partition. And the same is for arg "suspend" in kernel cmd line. > The way I see things, would it be possible (and not a pain) to make the > UUID part of the partition string passed to 'gpt write' optional. If > not passed, generate the UUIDs needed. What was used would be seen in > 'part list' and so forth. > > - -- > Tom Ok, so I remove saveenv() from my changes and then we will have two cases: # gpt write mmc 0 $partitions case 1: envorinment uuids are not set; then: proper uuids variables are set automatically (and printed) case 2: environment uuids are set in env (e.g. some user has put his own env); then users uuids will be used and new uuids are not generated automatically So this will not change current "gpt" usability - just add new feature, moreover user will be informed about each uuid generation. In case when someone use gpt write by mistake and overwrite uuids by randomly generated then he can easily back to his own uuids by setting each in environment and run gpt write again. > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJTFI32AAoJENk4IS6UOR1WAyQP/2bfClvRaWtLzUDggRUnNr99 > RKSgB1m/nl09ZwclB2Gn1FZL2IR/M0u+ia89FE9leSN4eoOEswN2rrUiExVv7QpJ > d+oCH/H3oNDAyb9L4HXXYHeRGCVHK7KE/KP5Ngb7KfTGZhj5kHdkx9iQM7dO9OtX > DmvW5+JoRXgXPkpmvy5s20IzRUbBEqGC6Z2h8Y/VKHTDnrUmYvFb6XPgjwwrYVB6 > +Hx/7K2R1uXQTuoHM7FqVAqKx7GGiUmcpLG90iHeYcX4fib/+RwC7hcre0rRj71v > NyAnO4t0nXKefLXYe5iGold7cNx9xUKO3s4u2EkC9lNGRkN6RcVuaxzEKGD4/QTX > fHA2q6FwSVLv4lXBT6UWxXiky2C9TMY/DUNM0EWe2KSx2V2glXPKiBX7gRk0Rq0d > EWq1G5oQS0qiZgb+vUQ4Acf4/HDjhAl1l18Hx6w+26LNvz8Fi+o7Om4kkBdI5sos > GX2auS1RxQc3qQkrAjxFx1xpDr9iMikV6nNqYDcVU894PeCgzmUWaWG/IrBFixO6 > 2XjF1sMth8LrVqUHirh3Y7lDU+FFPP+mMb6eaY3oajtVkg3u6cpQ0eJ0A612CsXG > oaTPxWKKTsWlBxaVIgzu2OXeYg5BJF4OTKxjNiV2wptheEVc4RVLDiQ1yf74YEHy > Hve7lpxa9i4Oo9Vf2Pd7 > =5FKf > -----END PGP SIGNATURE----- > Thank you,
On Mon, Mar 03, 2014 at 04:31:35PM +0100, Przemyslaw Marczak wrote: > Hello Tom, > > On 03/03/2014 03:13 PM, Tom Rini wrote: > >-----BEGIN PGP SIGNED MESSAGE----- > >Hash: SHA1 > > > >On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: > > > >[snip] > >>Actually automatically generated uuids was the main purpose of this > >>patches. Setting each env variable in this place was the most easy way > >>to make this without a lot of duplicated code. > >> > >>Why do you treat it like a side-effect? > >>If user wants have own generated uuids - then he can manually set env > >>variables like "uuid_gpt_disk". > >>This actually is not changed - when uuid env is set then it will be used > >>like in current version of code. > >>When user can't generate uuids or just wants to have it automatically > >>generated then my code do this job. > > > >Having been using this code again myself recently, at the high level, > >this is useful. Having to copy/paste in N UUIDs gets silly. But I also > >wonder.. > > > >[snip] > >>The one and only reason for put saveenv() here was that if uuids are > >>randomly generated or even just are in environment then I can be sure > >>that next gpt write (e.g. in case of overwrite sector 0 by mistake) is > >>using the same uuids values. > > > >Is this really an important use case to cover? > > It can be important if somebody uses UUIDS to boot kernel. In kernel > documentation you can find a notice about kernel function > name_to_dev_t() - so by command line you can pass uuid for root > partition. And the same is for arg "suspend" in kernel cmd line. Right. But that seems to be putting things in the wrong order. If you need to restore UUIDs to your partition table, you pass in the optional and already known UUID. If you're starting from scratch, by the time the installer is run U-Boot is long gone. And tying things back to the commodity distro stuff, we would be fetching 'root=UUID=...' from some file generated and controlled on the Linux side of things anyhow. To be clear, on the OS side of the equation there's much better ways to find out that partition1 has a UUID of ... than poking the U-Boot environment. > >The way I see things, would it be possible (and not a pain) to make the > >UUID part of the partition string passed to 'gpt write' optional. If > >not passed, generate the UUIDs needed. What was used would be seen in > >'part list' and so forth. > > Ok, so I remove saveenv() from my changes and then we will have two cases: > > # gpt write mmc 0 $partitions > case 1: envorinment uuids are not set; then: > proper uuids variables are set automatically (and printed) I'd go so far as to say we don't need to print the uuids, they're available via 'part list ...'. A notice that we're generating UUIDs is probably not too spammy. > case 2: environment uuids are set in env (e.g. some user has put his > own env); then > users uuids will be used and new uuids are not generated automatically > > So this will not change current "gpt" usability - just add new > feature, moreover user will be informed about each uuid generation. > In case when someone use gpt write by mistake and overwrite uuids by > randomly generated then he can easily back to his own uuids by > setting each in environment and run gpt write again. Right. We're making the use case of "fresh device, create new table" easier and we're still allowing "existing device, re-establish old UUIDs".
On 03/03/2014 05:46 PM, Tom Rini wrote: > On Mon, Mar 03, 2014 at 04:31:35PM +0100, Przemyslaw Marczak wrote: >> Hello Tom, >> >> On 03/03/2014 03:13 PM, Tom Rini wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: >>> >>> [snip] >>>> Actually automatically generated uuids was the main purpose of this >>>> patches. Setting each env variable in this place was the most easy way >>>> to make this without a lot of duplicated code. >>>> >>>> Why do you treat it like a side-effect? >>>> If user wants have own generated uuids - then he can manually set env >>>> variables like "uuid_gpt_disk". >>>> This actually is not changed - when uuid env is set then it will be used >>>> like in current version of code. >>>> When user can't generate uuids or just wants to have it automatically >>>> generated then my code do this job. >>> >>> Having been using this code again myself recently, at the high level, >>> this is useful. Having to copy/paste in N UUIDs gets silly. But I also >>> wonder.. >>> >>> [snip] >>>> The one and only reason for put saveenv() here was that if uuids are >>>> randomly generated or even just are in environment then I can be sure >>>> that next gpt write (e.g. in case of overwrite sector 0 by mistake) is >>>> using the same uuids values. >>> >>> Is this really an important use case to cover? >> >> It can be important if somebody uses UUIDS to boot kernel. In kernel >> documentation you can find a notice about kernel function >> name_to_dev_t() - so by command line you can pass uuid for root >> partition. And the same is for arg "suspend" in kernel cmd line. > > Right. But that seems to be putting things in the wrong order. If you > need to restore UUIDs to your partition table, you pass in the optional > and already known UUID. If you're starting from scratch, by the time > the installer is run U-Boot is long gone. And tying things back to the > commodity distro stuff, we would be fetching 'root=UUID=...' from some > file generated and controlled on the Linux side of things anyhow. To be > clear, on the OS side of the equation there's much better ways to find > out that partition1 has a UUID of ... than poking the U-Boot > environment. > Ok, I understand this. >>> The way I see things, would it be possible (and not a pain) to make the >>> UUID part of the partition string passed to 'gpt write' optional. If >>> not passed, generate the UUIDs needed. What was used would be seen in >>> 'part list' and so forth. >> >> Ok, so I remove saveenv() from my changes and then we will have two cases: >> >> # gpt write mmc 0 $partitions >> case 1: envorinment uuids are not set; then: >> proper uuids variables are set automatically (and printed) > > I'd go so far as to say we don't need to print the uuids, they're > available via 'part list ...'. A notice that we're generating UUIDs is > probably not too spammy. > Ok, I will remove this notice. >> case 2: environment uuids are set in env (e.g. some user has put his >> own env); then >> users uuids will be used and new uuids are not generated automatically >> >> So this will not change current "gpt" usability - just add new >> feature, moreover user will be informed about each uuid generation. >> In case when someone use gpt write by mistake and overwrite uuids by >> randomly generated then he can easily back to his own uuids by >> setting each in environment and run gpt write again. > > Right. We're making the use case of "fresh device, create new table" > easier and we're still allowing "existing device, re-establish old > UUIDs". > That's the point. > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Thanks,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/03/2014 12:23 PM, Przemyslaw Marczak wrote: > On 03/03/2014 05:46 PM, Tom Rini wrote: >> On Mon, Mar 03, 2014 at 04:31:35PM +0100, Przemyslaw Marczak wrote: >>> Hello Tom, >>> >>> On 03/03/2014 03:13 PM, Tom Rini wrote: >>>> -----BEGIN PGP SIGNED MESSAGE----- >>>> Hash: SHA1 >>>> >>>> On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: >>>> >>>> [snip] >>>>> Actually automatically generated uuids was the main purpose of this >>>>> patches. Setting each env variable in this place was the most easy way >>>>> to make this without a lot of duplicated code. >>>>> >>>>> Why do you treat it like a side-effect? >>>>> If user wants have own generated uuids - then he can manually set env >>>>> variables like "uuid_gpt_disk". >>>>> This actually is not changed - when uuid env is set then it will be >>>>> used >>>>> like in current version of code. >>>>> When user can't generate uuids or just wants to have it automatically >>>>> generated then my code do this job. >>>> >>>> Having been using this code again myself recently, at the high level, >>>> this is useful. Having to copy/paste in N UUIDs gets silly. But I >>>> also >>>> wonder.. >>>> >>>> [snip] >>>>> The one and only reason for put saveenv() here was that if uuids are >>>>> randomly generated or even just are in environment then I can be sure >>>>> that next gpt write (e.g. in case of overwrite sector 0 by mistake) is >>>>> using the same uuids values. >>>> >>>> Is this really an important use case to cover? >>> >>> It can be important if somebody uses UUIDS to boot kernel. In kernel >>> documentation you can find a notice about kernel function >>> name_to_dev_t() - so by command line you can pass uuid for root >>> partition. And the same is for arg "suspend" in kernel cmd line. >> >> Right. But that seems to be putting things in the wrong order. If you >> need to restore UUIDs to your partition table, you pass in the optional >> and already known UUID. If you're starting from scratch, by the time >> the installer is run U-Boot is long gone. And tying things back to the >> commodity distro stuff, we would be fetching 'root=UUID=...' from some >> file generated and controlled on the Linux side of things anyhow. To be >> clear, on the OS side of the equation there's much better ways to find >> out that partition1 has a UUID of ... than poking the U-Boot >> environment. >> > > Ok, I understand this. > >>>> The way I see things, would it be possible (and not a pain) to make the >>>> UUID part of the partition string passed to 'gpt write' optional. If >>>> not passed, generate the UUIDs needed. What was used would be seen in >>>> 'part list' and so forth. >>> >>> Ok, so I remove saveenv() from my changes and then we will have two >>> cases: >>> >>> # gpt write mmc 0 $partitions >>> case 1: envorinment uuids are not set; then: >>> proper uuids variables are set automatically (and printed) >> >> I'd go so far as to say we don't need to print the uuids, they're >> available via 'part list ...'. A notice that we're generating UUIDs is >> probably not too spammy. >> > > Ok, I will remove this notice. > >>> case 2: environment uuids are set in env (e.g. some user has put his >>> own env); then >>> users uuids will be used and new uuids are not generated >>> automatically >>> >>> So this will not change current "gpt" usability - just add new >>> feature, moreover user will be informed about each uuid generation. >>> In case when someone use gpt write by mistake and overwrite uuids by >>> randomly generated then he can easily back to his own uuids by >>> setting each in environment and run gpt write again. >> >> Right. We're making the use case of "fresh device, create new table" >> easier and we're still allowing "existing device, re-establish old >> UUIDs". >> > That's the point. Yay for agreement then, even if it took a few rounds to see we're on the same side :) I look forward to v2. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJTFL1DAAoJENk4IS6UOR1WT0EP/12dTZmJGcgZYPgIaRohWAjS Dk3n0PdkPszGic/n5j6un6b8gSck27cTI3aM5YvqRioRBTzw6tvGVoIBOfrtEKdZ EV78iqMq1cozL3o3VdltQlLAzdLzSMwkkEqdD9K4vWpihdr19PPPuaPlV0ynw4/D 6+a1azEqTtznx96bGIH0XLK37ZtVre0/n1+9nd+mQ/swibHQM84GNPdR1L5m3hnJ W80XCnX4wRh238KxXRKr/fqG5w9cVXFcsPWESXzfA+pzi9Jy37dkhMX3CyAGuwS0 QDLz1gg4h6CYGCRulN6rfNYHbLg8YYbJ/5xa4/dSntUW84uZJRMq93T9yKYX+qKM Iezp+EKzoL3Lagz3K2GmwopazEd+0goEHauoQ8QUGDdgMGapXui5NBUNyke78JK8 U/WszX6JOWjagenWHDYiBvPhwIWdGw3FUJVijutKhK8KpKKihC9eDOsdoxz+Lyfu agcZ8rtJ8s6QBEf+xo/PRS+twJeHuTe/CptORm52cXxPMrwOKK17UxrjLJIuDSDl fqVsUaUHmxFmUNU/k5SGI60d/nZGmi7j8EzMMRIK8DJ18Sk0FaBOj/PKo2xBf3RO afbnEQXi831j42V2HSOEEY6rKRzES9d0roSUFIe5EvP/s/1/O1OanimtrXRISjF9 rKXW1zAJZKQlBbYoDOSa =eGvG -----END PGP SIGNATURE-----
On 03/03/2014 06:35 PM, Tom Rini wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 03/03/2014 12:23 PM, Przemyslaw Marczak wrote: >> On 03/03/2014 05:46 PM, Tom Rini wrote: >>> On Mon, Mar 03, 2014 at 04:31:35PM +0100, Przemyslaw Marczak wrote: >>>> Hello Tom, >>>> >>>> On 03/03/2014 03:13 PM, Tom Rini wrote: >>>>> -----BEGIN PGP SIGNED MESSAGE----- >>>>> Hash: SHA1 >>>>> >>>>> On 03/03/2014 08:45 AM, Przemyslaw Marczak wrote: >>>>> >>>>> [snip] >>>>>> Actually automatically generated uuids was the main purpose of this >>>>>> patches. Setting each env variable in this place was the most easy way >>>>>> to make this without a lot of duplicated code. >>>>>> >>>>>> Why do you treat it like a side-effect? >>>>>> If user wants have own generated uuids - then he can manually set env >>>>>> variables like "uuid_gpt_disk". >>>>>> This actually is not changed - when uuid env is set then it will be >>>>>> used >>>>>> like in current version of code. >>>>>> When user can't generate uuids or just wants to have it automatically >>>>>> generated then my code do this job. >>>>> >>>>> Having been using this code again myself recently, at the high level, >>>>> this is useful. Having to copy/paste in N UUIDs gets silly. But I >>>>> also >>>>> wonder.. >>>>> >>>>> [snip] >>>>>> The one and only reason for put saveenv() here was that if uuids are >>>>>> randomly generated or even just are in environment then I can be sure >>>>>> that next gpt write (e.g. in case of overwrite sector 0 by mistake) is >>>>>> using the same uuids values. >>>>> >>>>> Is this really an important use case to cover? >>>> >>>> It can be important if somebody uses UUIDS to boot kernel. In kernel >>>> documentation you can find a notice about kernel function >>>> name_to_dev_t() - so by command line you can pass uuid for root >>>> partition. And the same is for arg "suspend" in kernel cmd line. >>> >>> Right. But that seems to be putting things in the wrong order. If you >>> need to restore UUIDs to your partition table, you pass in the optional >>> and already known UUID. If you're starting from scratch, by the time >>> the installer is run U-Boot is long gone. And tying things back to the >>> commodity distro stuff, we would be fetching 'root=UUID=...' from some >>> file generated and controlled on the Linux side of things anyhow. To be >>> clear, on the OS side of the equation there's much better ways to find >>> out that partition1 has a UUID of ... than poking the U-Boot >>> environment. >>> >> >> Ok, I understand this. >> >>>>> The way I see things, would it be possible (and not a pain) to make the >>>>> UUID part of the partition string passed to 'gpt write' optional. If >>>>> not passed, generate the UUIDs needed. What was used would be seen in >>>>> 'part list' and so forth. >>>> >>>> Ok, so I remove saveenv() from my changes and then we will have two >>>> cases: >>>> >>>> # gpt write mmc 0 $partitions >>>> case 1: envorinment uuids are not set; then: >>>> proper uuids variables are set automatically (and printed) >>> >>> I'd go so far as to say we don't need to print the uuids, they're >>> available via 'part list ...'. A notice that we're generating UUIDs is >>> probably not too spammy. >>> >> >> Ok, I will remove this notice. >> >>>> case 2: environment uuids are set in env (e.g. some user has put his >>>> own env); then >>>> users uuids will be used and new uuids are not generated >>>> automatically >>>> >>>> So this will not change current "gpt" usability - just add new >>>> feature, moreover user will be informed about each uuid generation. >>>> In case when someone use gpt write by mistake and overwrite uuids by >>>> randomly generated then he can easily back to his own uuids by >>>> setting each in environment and run gpt write again. >>> >>> Right. We're making the use case of "fresh device, create new table" >>> easier and we're still allowing "existing device, re-establish old >>> UUIDs". >>> >> That's the point. > > Yay for agreement then, even if it took a few rounds to see we're on the > same side :) I look forward to v2. > > - -- > Tom > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJTFL1DAAoJENk4IS6UOR1WT0EP/12dTZmJGcgZYPgIaRohWAjS > Dk3n0PdkPszGic/n5j6un6b8gSck27cTI3aM5YvqRioRBTzw6tvGVoIBOfrtEKdZ > EV78iqMq1cozL3o3VdltQlLAzdLzSMwkkEqdD9K4vWpihdr19PPPuaPlV0ynw4/D > 6+a1azEqTtznx96bGIH0XLK37ZtVre0/n1+9nd+mQ/swibHQM84GNPdR1L5m3hnJ > W80XCnX4wRh238KxXRKr/fqG5w9cVXFcsPWESXzfA+pzi9Jy37dkhMX3CyAGuwS0 > QDLz1gg4h6CYGCRulN6rfNYHbLg8YYbJ/5xa4/dSntUW84uZJRMq93T9yKYX+qKM > Iezp+EKzoL3Lagz3K2GmwopazEd+0goEHauoQ8QUGDdgMGapXui5NBUNyke78JK8 > U/WszX6JOWjagenWHDYiBvPhwIWdGw3FUJVijutKhK8KpKKihC9eDOsdoxz+Lyfu > agcZ8rtJ8s6QBEf+xo/PRS+twJeHuTe/CptORm52cXxPMrwOKK17UxrjLJIuDSDl > fqVsUaUHmxFmUNU/k5SGI60d/nZGmi7j8EzMMRIK8DJ18Sk0FaBOj/PKo2xBf3RO > afbnEQXi831j42V2HSOEEY6rKRzES9d0roSUFIe5EvP/s/1/O1OanimtrXRISjF9 > rKXW1zAJZKQlBbYoDOSa > =eGvG > -----END PGP SIGNATURE----- > I hope to send V2 tomorrow :) Regards
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index 1f12e6d..7be5fcf 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -31,6 +31,7 @@ */ static char extract_env(const char *str, char **env) { + int ret = -1; char *e, *s; if (!str || strlen(str) < 4) @@ -43,16 +44,23 @@ static char extract_env(const char *str, char **env) memset(s + strlen(s) - 1, '\0', 1); memmove(s, s + 2, strlen(s) - 1); e = getenv(s); - free(s); if (e == NULL) { - printf("Environmental '%s' not set\n", str); - return -1; /* env not set */ + printf("%s unset. ", str); + e = get_uuid_str(); + if (e) { + printf("Setting to: %s\n", e); + setenv(s, e); + ret = 0; + } + } else { + ret = 0; } + *env = e; - return 0; + free(s); } - return -1; + return ret; } /** @@ -299,8 +307,17 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return CMD_RET_FAILURE; } - if (gpt_default(blk_dev_desc, argv[4])) + puts("Writing GPT: "); + + ret = gpt_default(blk_dev_desc, argv[4]); + if (!ret) { + puts("success!\n"); + saveenv(); + return CMD_RET_SUCCESS; + } else { + puts("error!\n"); return CMD_RET_FAILURE; + } } else { return CMD_RET_USAGE; } diff --git a/doc/README.gpt b/doc/README.gpt index 5c133f3..afe2538 100644 --- a/doc/README.gpt +++ b/doc/README.gpt @@ -176,3 +176,4 @@ Please, pay attention at -l switch for parted. "uuid" program is recommended to generate UUID string. Moreover it can decode (-d switch) passed in UUID string. It can be used to generate partitions UUID passed to u-boot environment variables. +If each partition "uuid" no exists then it will be randomly generated.