New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 904539 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug

Blocking:
issue 901893



Sign in to add a comment

move libbrillo/imageloader/manifest* back to imageloader and make it a shared library

Project Member Reported by xiaochu@chromium.org, Nov 12

Issue description

Based 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.
 
Blocking: 901893
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?
Shared lib makes sense to me if this is just for parsing a file.
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...
awesome. thanks!
Summary: move libbrillo/imageloader/manifest* back to imageloader and make it a shared library (was: move libbrillo/imageloader/manifest* back to imageloader)
Labels: Pri-2
Project Member

Comment 9 by bugdroid1@chromium.org, 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

Cc: chowes@google.com
 Issue 914902  has been merged into this issue.
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.
Re storing numbers in JSON, please read https://stackoverflow.com/questions/13502398/json-integers-limit-on-size (or other similar resources).
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Project Member

Comment 14 by bugdroid1@chromium.org, 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

Project Member

Comment 15 by bugdroid1@chromium.org, 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

Status: Assigned (was: Untriaged)
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