move libbrillo/imageloader/manifest* back to imageloader and make it a shared library |
||||
Issue descriptionBased on discussions in https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1323759 and https://bugs.chromium.org/p/chromium/issues/detail?id=890036, it's not recommended to put imageloader code in libbrillo. However, we still have to expose the logic to dlcservice. We'll add a new imageloader d-bus method GetMetadata (name TBD) which returns a metadata protobuf. So let's move libbrillo/imageloader to imageloader first.
,
Nov 13
Or instead of adding complicated dbus signals, they could just use a shared library (exported by imageloader) to do the things they both do, but on their own terms. Isn't it just parsing a manifest? can't that be just a shared piece of library instead of a dbus signal?
,
Nov 13
Shared lib makes sense to me if this is just for parsing a file.
,
Nov 13
shared lib sounds good to me. is there an example in platform2 to export a normal library? i'm only aware of special libraries like protobuf and d-bus client...
,
Nov 13
Just search for shared_library in platform2: https://cs.corp.google.com/search/?q=shared_library+file:%5Esrc/platform2/+package:%5Echromeos_public$&m=25&type=cs
,
Nov 13
awesome. thanks!
,
Nov 13
,
Nov 14
,
Nov 16
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/e61e1d6daa5a1b94fedcd9b48188e9a323415f85 commit e61e1d6daa5a1b94fedcd9b48188e9a323415f85 Author: Xiaochu Liu <xiaochu@chromium.org> Date: Fri Nov 16 02:49:27 2018 imageloader: move manifest* from libbrillo to imageloader This is the first part of the migration: copy manifest* to imageloader and remove imageloader's dependencies to libbrillo/imageloader. BUG=chromium:904539 TEST=FEATURES="test" emerge-kefka imageloader Change-Id: Ie711c687c85e6da637a462ce922fd12f6de9081f Reviewed-on: https://chromium-review.googlesource.com/1332387 Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com> Tested-by: Xiaochu Liu <xiaochu@chromium.org> Reviewed-by: Amin Hassani <ahassani@chromium.org> [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/helper_process_proxy.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/helper_process_proxy.h [rename] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/manifest.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/component_test.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/imageloader_test.cc [rename] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/manifest.h [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/dlc_test.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/component.h [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/imageloader.gyp [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/dlc.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/component.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/mock_helper_process_proxy.h [rename] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/imageloader/manifest_test.cc [modify] https://crrev.com/e61e1d6daa5a1b94fedcd9b48188e9a323415f85/libbrillo/libbrillo.gypi
,
Dec 13
,
Dec 14
I'm writing a patch set to move the manifest parser into a shared library with support for the fields we need to grab out of DLC. A potential issue I see here is that the DictionaryValue class that we use for parsing the manifest doesn't support grabbing integers other than 'int', so we could see overflows for files >= 2 GiB when we get the preallocated size and partition size (assuming int is 32 bits). derat@ proposed storing these values as strings, which would solve this problem.
,
Dec 14
Re storing numbers in JSON, please read https://stackoverflow.com/questions/13502398/json-integers-limit-on-size (or other similar resources).
,
Dec 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/dc15ba2489e0483ee319473ba4f3906348cd43e2 commit dc15ba2489e0483ee319473ba4f3906348cd43e2 Author: Colin Howes <chowes@google.com> Date: Sat Dec 15 06:40:19 2018 imageloader: Export manifest parsing functions as a shared library We need to read the imageloader manifest files in /opt/google/dlc in a few different places. Create a shared library so we don't duplicate code. Also add fields relevant to DLC images - see dlcservice and chromite/scripts/build_dlc.py. BUG=chromium:904539 TEST=Install a DLC and check that imageloader works as before. CQ-DEPEND=CL:1376135,CL:1376136 Change-Id: I8608970f9943b87c278143016c131366d2f4fb02 Reviewed-on: https://chromium-review.googlesource.com/1376377 Commit-Ready: Colin Howes <chowes@google.com> Tested-by: Colin Howes <chowes@google.com> Reviewed-by: Greg Kerr <kerrnel@chromium.org> Reviewed-by: Dan Erat <derat@chromium.org> [add] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/manifest.md [add] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/libimageloader-manifest.pc [modify] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/manifest.h [modify] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/manifest.cc [modify] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/BUILD.gn [modify] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/imageloader/manifest_test.cc [modify] https://crrev.com/dc15ba2489e0483ee319473ba4f3906348cd43e2/dlcservice/BUILD.gn
,
Dec 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/034c9775ebfa0f138ebb7c963e8d123e08162b22 commit 034c9775ebfa0f138ebb7c963e8d123e08162b22 Author: Colin Howes <chowes@google.com> Date: Sat Dec 15 06:40:19 2018 imageloader: Export manifest parser as a shared library. We parse imageloader manifest files in a few places. Export this functionality as a shared library so we can use it in dlcservice. BUG=chromium:904539 TEST=Install a DLC, everything works as before. CQ-DEPEND=CL:1376377 Change-Id: Icf57713a4a14c9df5f4ce4f096e3a9a0fb475079 Reviewed-on: https://chromium-review.googlesource.com/1376135 Commit-Ready: Colin Howes <chowes@google.com> Tested-by: Colin Howes <chowes@google.com> Reviewed-by: Amin Hassani <ahassani@chromium.org> Reviewed-by: Nicolas Norvez <norvez@chromium.org> [modify] https://crrev.com/034c9775ebfa0f138ebb7c963e8d123e08162b22/chromeos-base/imageloader/imageloader-9999.ebuild
,
Dec 15
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/9f81949bae87a71f375001e0cbb38ce9a63c4cf2 commit 9f81949bae87a71f375001e0cbb38ce9a63c4cf2 Author: Colin Howes <chowes@google.com> Date: Sat Dec 15 06:40:20 2018 dlcservice: Depend on imageloader. We need to depend on imageloader in order to use the manifest parsing shared library. BUG=chromium:904539 TEST=Build dlcservice using the new shared library. Change-Id: I90758bcc8e8be89d9db58fdb5ca141a3133231ba Reviewed-on: https://chromium-review.googlesource.com/1376136 Commit-Ready: Colin Howes <chowes@google.com> Tested-by: Colin Howes <chowes@google.com> Reviewed-by: Amin Hassani <ahassani@chromium.org> Reviewed-by: Nicolas Norvez <norvez@chromium.org> [modify] https://crrev.com/9f81949bae87a71f375001e0cbb38ce9a63c4cf2/chromeos-base/dlcservice/dlcservice-9999.ebuild
,
Jan 11
This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this. |
||||
►
Sign in to add a comment |
||||
Comment 1 by xiaochu@chromium.org
, Nov 12