Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1736493002:
Update VPD cache file location. (Closed)
|
Created:
4 years, 10 months ago by Thiemo Nagel Modified:
3 years, 7 months ago CC:
chromium-reviews, oshima+watch_chromium.org, Mattias Nissler (ping if slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate VPD cache file location.
In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files
have been migrated to unencrypted stateful and symlinks have been placed
in the original locations for backward compatibility.
This CL updates the file locations inside Chrome so that we may
eventually get rid of the symlinks and can drop the exceptions for them,
cf. https://crbug.com/655606.
BUG=none
Review-Url: https://codereview.chromium.org/1736493002
Cr-Commit-Position: refs/heads/master@{#467645}
Committed: https://chromium.googlesource.com/chromium/src/+/e62605ff521363439ef54d294472e193d6cba47d
Patch Set 1 #Patch Set 2 : Rebase #
Messages
Total messages: 34 (16 generated)
tnagel@chromium.org changed reviewers: + gauravsh@chromium.org
Gaurav, since you did the underlying change, may I ask you to take a look at this CL? Thank you! Thiemo
LGTM Have you tested this? I don't remember now but there may have been a reason why we didn't do this in the first place... On 2016/02/24 15:25:56, Thiemo Nagel wrote: > Gaurav, since you did the underlying change, may I ask you to take a look at > this CL? > > Thank you! > Thiemo
Thank you! On 2016/02/24 17:44:35, gauravsh wrote: > Have you tested this? I don't remember now but there may have been a reason why > we didn't do this in the first place... Not yet. Will do.
Successfully tested enrollment (which depends on VPD). +hungte FYI
tnagel@chromium.org changed reviewers: + derat@chromium.org
Hi Dan, may I ask for OWNER review? Thank you! Thiemo
lgtm
On 2016/03/01 19:09:43, Daniel Erat wrote: > lgtm Thank you!
The CQ bit was checked by tnagel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1736493002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1736493002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
tnagel@chromium.org changed reviewers: + hungte@chromium.org
Moving that discussion to the code review. Hung-Te wrote: > It seems ok to me to change log/vpd_2.0.txt to cache/vpd/filtered.txt, but I'm not sure if it's a good idea to replace > /var by /mnt/stateful_partition/unencrypted​. /var/cache/vpd/filtered.txt doesn't exist at the moment. > Isn't it better to keep "logical" path instead of "physical location" so we don't need to change Chrome code again if > someday people decides to move vpd data out of unencrypted folder, or on systems without unencrypted folder (but still > have vpd in /var)? I guess I had interpreted the choice of words "migrate" and "legacy" as an indication that we would like to get rid of the old files eventually. My aim was to move forward in that direction in order to reduce complexity in dump_vpd_log. If you think that the symlinks are a useful abstraction, that's fine with me, too. (Maybe in that case we should clarify the comments in dump_vpd_log a bit.) Cheers, Thiemo
I guess there's no clear benefit of this CL. --> abandoning
Message was sent while issue was closed.
Hi Thiemo, seems like I missed this discussion. Sorry but I have changed my mind. Creating symlink in dump_vpd_log does create extra logic and execution time, also the problem of file directory permissions - that we already have other scripts using unencrypted folders directly. Original VPD cache owner (yjou) has left so I think it's fine if you think changing the path is better. We can change the path anytime if needed, since vpd.conf is executed on every boot.
Message was sent while issue was closed.
Description was changed from ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. Updating the file locations inside Chrome so that we might (eventually) get rid of the symlinks. BUG=None ========== to ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. Updating the file locations inside Chrome so that we might (eventually) get rid of the symlinks. BUG=None ==========
> Sorry but I have changed my mind. Creating symlink in dump_vpd_log does create > extra logic and execution time, also the problem of file directory permissions - > that we already have other scripts using unencrypted folders directly. Alright, re-opening the CL, also in the light of https://crbug.com/655606.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. Updating the file locations inside Chrome so that we might (eventually) get rid of the symlinks. BUG=None ========== to ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. This CL updates the file locations inside Chrome so that we may eventually get rid of the symlinks and can drop the exceptions for them, cf. https://crbug.com/655606. BUG=none ==========
Dan, as it's been a year, may I kindly ask you to re-review and affirm your l-g-t-m?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm; also cc-ing mattias
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
LGTM, thanks for doing this!
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gauravsh@chromium.org Link to the patchset: https://codereview.chromium.org/1736493002/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493295287622530,
"parent_rev": "19c029857590d6de09de4fee67c92a9d457391c4", "commit_rev":
"e62605ff521363439ef54d294472e193d6cba47d"}
Message was sent while issue was closed.
Description was changed from ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. This CL updates the file locations inside Chrome so that we may eventually get rid of the symlinks and can drop the exceptions for them, cf. https://crbug.com/655606. BUG=none ========== to ========== Update VPD cache file location. In https://chromium-review.googlesource.com/#/c/32606/ VPD cache files have been migrated to unencrypted stateful and symlinks have been placed in the original locations for backward compatibility. This CL updates the file locations inside Chrome so that we may eventually get rid of the symlinks and can drop the exceptions for them, cf. https://crbug.com/655606. BUG=none Review-Url: https://codereview.chromium.org/1736493002 Cr-Commit-Position: refs/heads/master@{#467645} Committed: https://chromium.googlesource.com/chromium/src/+/e62605ff521363439ef54d294472... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/e62605ff521363439ef54d294472... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
