| Message ID | 20190815200403.1152-2-semen.protsenko@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8f684b5a8b8351bf2d464609071ccd2de35c8773 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,1/2] libavb: Update libavb to current AOSP master | expand |
Hi Sam, CC: LIBAVB people (w.r.t. libavb fixes in U-Boot) I can reproduce the compiler warnings myself and I confirm they are fixed with this patch. More comments below. On Thu, Aug 15, 2019 at 11:04:03PM +0300, Sam Protsenko wrote: > After updating libavb to most recent version from AOSP/master, two new > warnings appear: > > Warning #1: > > lib/libavb/avb_cmdline.c: In function 'avb_append_options': > lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be > used uninitialized in this function > [-Wmaybe-uninitialized] > new_ret = avb_replace( > ^~~~~~~~~~~~ > slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used > uninitialized in this function > [-Wmaybe-uninitialized] > if (!cmdline_append_option( > ^~~~~~~~~~~~~~~~~~~~~~ > slot_data, "androidboot.veritymode", verity_mode)) { > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Warning #2: > > lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify': > lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used > uninitialized in this function > [-Wmaybe-uninitialized] > AvbSlotVerifyResult ret; > ^~~ FWIW, few of Linux commits do word-wrapping of ASCII/console dumps for the sake of improved readability and git grepping. Recent checkpatch versions don't warn on that. > > Fix those by providing default return values to affected functions. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > --- > lib/libavb/avb_cmdline.c | 3 ++- > lib/libavb/avb_slot_verify.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c > index cb5b98e423..684c512bb9 100644 > --- a/lib/libavb/avb_cmdline.c > +++ b/lib/libavb/avb_cmdline.c > @@ -357,7 +357,8 @@ AvbSlotVerifyResult avb_append_options( > // Should never get here because MANAGED_RESTART_AND_EIO is > // remapped by avb_manage_hashtree_error_mode(). > avb_assert_not_reached(); > - break; > + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; > + goto out; With AVB_ENABLE_DEBUG enabled, PVS-Studio reports: lib/libavb/avb_cmdline.c 360 warn V779 Unreachable code detected. It is possible that an error is present. How about replacing the 'break' statement with a 'fallthrough' comment? It shuts down the warning w/o changing the functionality. > default: > ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; > goto out; > diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c > index 5d400b38aa..c0defdf9c9 100644 > --- a/lib/libavb/avb_slot_verify.c > +++ b/lib/libavb/avb_slot_verify.c > @@ -1346,7 +1346,7 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, > AvbSlotVerifyFlags flags, > AvbHashtreeErrorMode hashtree_error_mode, > AvbSlotVerifyData** out_data) { > - AvbSlotVerifyResult ret; > + AvbSlotVerifyResult ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; What should we do with these out-of-tree libavb fixes? IMHO they are not specific to U-Boot and should be upstream-able. IMHO it is not healthy to accumulate too many of them, since this will make future libavb sync more painful.
Hi, As for upstreaming libavb patches, I'd be interested in landing them upstream... makes it easier for anyone. Our upstream is AOSP and we use gerrit for code-review: https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb Here's a guide to contributing: https://source.android.com/setup/contribute/submit-patches ... hope it's not too painful to use the AOSP process! Thanks, David On Fri, Aug 16, 2019 at 9:35 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > Hi Sam, > > CC: LIBAVB people (w.r.t. libavb fixes in U-Boot) > > I can reproduce the compiler warnings myself and I confirm they are > fixed with this patch. More comments below. > > On Thu, Aug 15, 2019 at 11:04:03PM +0300, Sam Protsenko wrote: > > After updating libavb to most recent version from AOSP/master, two new > > warnings appear: > > > > Warning #1: > > > > lib/libavb/avb_cmdline.c: In function 'avb_append_options': > > lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be > > used uninitialized in this function > > [-Wmaybe-uninitialized] > > new_ret = avb_replace( > > ^~~~~~~~~~~~ > > slot_data->cmdline, "$(ANDROID_VERITY_MODE)", > dm_verity_mode); > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used > > uninitialized in this function > > [-Wmaybe-uninitialized] > > if (!cmdline_append_option( > > ^~~~~~~~~~~~~~~~~~~~~~ > > slot_data, "androidboot.veritymode", verity_mode)) { > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > Warning #2: > > > > lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify': > > lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used > > uninitialized in this function > > [-Wmaybe-uninitialized] > > AvbSlotVerifyResult ret; > > ^~~ > > FWIW, few of Linux commits do word-wrapping of ASCII/console dumps for > the sake of improved readability and git grepping. Recent checkpatch > versions don't warn on that. > > > > > Fix those by providing default return values to affected functions. > > > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > > --- > > lib/libavb/avb_cmdline.c | 3 ++- > > lib/libavb/avb_slot_verify.c | 2 +- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c > > index cb5b98e423..684c512bb9 100644 > > --- a/lib/libavb/avb_cmdline.c > > +++ b/lib/libavb/avb_cmdline.c > > @@ -357,7 +357,8 @@ AvbSlotVerifyResult avb_append_options( > > // Should never get here because MANAGED_RESTART_AND_EIO is > > // remapped by avb_manage_hashtree_error_mode(). > > avb_assert_not_reached(); > > - break; > > + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; > > + goto out; > > With AVB_ENABLE_DEBUG enabled, PVS-Studio reports: > lib/libavb/avb_cmdline.c 360 warn V779 Unreachable code > detected. It is possible that an error is present. > > How about replacing the 'break' statement with a 'fallthrough' comment? > It shuts down the warning w/o changing the functionality. > > > default: > > ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; > > goto out; > > diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c > > index 5d400b38aa..c0defdf9c9 100644 > > --- a/lib/libavb/avb_slot_verify.c > > +++ b/lib/libavb/avb_slot_verify.c > > @@ -1346,7 +1346,7 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, > > AvbSlotVerifyFlags flags, > > AvbHashtreeErrorMode > hashtree_error_mode, > > AvbSlotVerifyData** out_data) { > > - AvbSlotVerifyResult ret; > > + AvbSlotVerifyResult ret = > AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; > > What should we do with these out-of-tree libavb fixes? IMHO they are not > specific to U-Boot and should be upstream-able. IMHO it is not healthy > to accumulate too many of them, since this will make future libavb sync > more painful. > > -- > Best Regards, > Eugeniu. >
Hi all, On Fri, Aug 16, 2019 at 11:46:03AM -0400, David Zeuthen wrote: > Hi, > As for upstreaming libavb patches, I'd be interested in landing them > upstream... makes it easier for anyone. > Our upstream is AOSP and we use gerrit for > code-review: https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb > Here's a guide to > contributing: https://source.android.com/setup/contribute/submit-patches ... > hope it's not too painful to use the AOSP process! > Thanks, > David David, thanks for being open about discussing/accepting the fixes. U-Boot folks, What's your opinion w.r.t. the right/best order between accepting the libavb fixes locally in U-Boot and pushing them to upstream? Which should come first? I suppose they can be first applied internally and replaced later on if they get reworked/optimized during upstreaming.
Hi Eugeniu, On Mon, Aug 19, 2019 at 11:28 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > Hi all, > > On Fri, Aug 16, 2019 at 11:46:03AM -0400, David Zeuthen wrote: > > Hi, > > As for upstreaming libavb patches, I'd be interested in landing them > > upstream... makes it easier for anyone. > > Our upstream is AOSP and we use gerrit for > > code-review: https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb > > Here's a guide to > > contributing: https://source.android.com/setup/contribute/submit-patches ... > > hope it's not too painful to use the AOSP process! > > Thanks, > > David > > David, thanks for being open about discussing/accepting the fixes. > > U-Boot folks, > > What's your opinion w.r.t. the right/best order between accepting > the libavb fixes locally in U-Boot and pushing them to upstream? > Which should come first? I suppose they can be first applied internally > and replaced later on if they get reworked/optimized during upstreaming. > > -- > Best Regards, > Eugeniu. IMHO, the best approach here is to upstream fixes to AOSP libavb first, only then port stuff to U-boot. Nevertheless, taking into account the amount of time needed (sometimes it can take months, or even years, Sam has good stories about that) to get something applied to AOSP, I would suggest to proceed internally with commits like this ASAP, but somehow (extended commit message for each "libavb sync" patch with the list of patches out of AOSP tree; or introduce "doc/android/libavb-porting.txt", where we can provide all details about list of commits from AOSP + out-of-tree commits, how-to do porting etc; other ideas?) keep track of changes that are not up-streamed to AOSP yet. Ideally we have to achieve 1:1 mapping (with the only difference - SPDX tags), of [1] in lib/libavb/, otherwise we will fight with constant code divergence (obviously). [1] https://android.googlesource.com/platform/external/avb/+/<revision>/libavb -- Best regards - Freundliche Grüsse - Meilleures salutations Igor Opaniuk mailto: igor.opaniuk@gmail.com skype: igor.opanyuk +380 (93) 836 40 67 http://ua.linkedin.com/in/iopaniuk
On Mon, Aug 19, 2019 at 12:40 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote: > > Hi Eugeniu, > > On Mon, Aug 19, 2019 at 11:28 AM Eugeniu Rosca <erosca@de.adit-jv.com> wrote: > > > > Hi all, > > > > On Fri, Aug 16, 2019 at 11:46:03AM -0400, David Zeuthen wrote: > > > Hi, > > > As for upstreaming libavb patches, I'd be interested in landing them > > > upstream... makes it easier for anyone. > > > Our upstream is AOSP and we use gerrit for > > > code-review: https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb > > > Here's a guide to > > > contributing: https://source.android.com/setup/contribute/submit-patches ... > > > hope it's not too painful to use the AOSP process! > > > Thanks, > > > David > > > > David, thanks for being open about discussing/accepting the fixes. > > > > U-Boot folks, > > > > What's your opinion w.r.t. the right/best order between accepting > > the libavb fixes locally in U-Boot and pushing them to upstream? > > Which should come first? I suppose they can be first applied internally > > and replaced later on if they get reworked/optimized during upstreaming. > > > > -- > > Best Regards, > > Eugeniu. > > IMHO, the best approach here is to upstream fixes to AOSP libavb first, > only then port stuff to U-boot. > > Nevertheless, taking into account the amount of time needed > (sometimes it can take months, or even years, Sam has good > stories about that) to get something applied to AOSP, I would suggest > to proceed internally with commits like this ASAP, but somehow (extended > commit message for each "libavb sync" patch with the list of patches out of > AOSP tree; or introduce "doc/android/libavb-porting.txt", where we can > provide all details about list of commits from AOSP + out-of-tree commits, > how-to do porting etc; other ideas?) keep track of changes that are not > up-streamed to AOSP yet. > > Ideally we have to achieve 1:1 mapping (with the only difference - SPDX tags), And avb_sysdeps_posix.c (implementation of avb_abort() and list of includes). You see, I'm already missing some of introduced changes on top of "vanilla" libavb. > of [1] in lib/libavb/, otherwise we will fight with constant code > divergence (obviously). > > [1] https://android.googlesource.com/platform/external/avb/+/<revision>/libavb > > -- > Best regards - Freundliche Grüsse - Meilleures salutations > > Igor Opaniuk > > mailto: igor.opaniuk@gmail.com > skype: igor.opanyuk > +380 (93) 836 40 67 > http://ua.linkedin.com/in/iopaniuk
On Mon, Aug 19, 2019 at 10:28:41AM +0200, Eugeniu Rosca wrote: > Hi all, > > On Fri, Aug 16, 2019 at 11:46:03AM -0400, David Zeuthen wrote: > > Hi, > > As for upstreaming libavb patches, I'd be interested in landing them > > upstream... makes it easier for anyone. > > Our upstream is AOSP and we use gerrit for > > code-review: https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb > > Here's a guide to > > contributing: https://source.android.com/setup/contribute/submit-patches ... > > hope it's not too painful to use the AOSP process! > > Thanks, > > David > > David, thanks for being open about discussing/accepting the fixes. > > U-Boot folks, > > What's your opinion w.r.t. the right/best order between accepting > the libavb fixes locally in U-Boot and pushing them to upstream? > Which should come first? I suppose they can be first applied internally > and replaced later on if they get reworked/optimized during upstreaming. I think we certainly need to submit the changes upstream first, and then keep our changes in-sync with any feedback we get from upstream. This is just like how we handle DTS changes too.
Hi, Just to re: > taking into account the amount of time needed (sometimes it can take months, or even years, Sam has good stories about that) to get something applied to AOSP This probably needs to be discussed case by case, as there are ~750 project <https://android.googlesource.com/platform/manifest/+/refs/heads/master/default.xml>s on AOSP now. For large changes, it might be helpful to start by a buganizer or e-mail thread, to reach a consensus with individual owners. For small changes (e.g., fix some code health issue), the review should be just a few days or a few weeks. On Mon, Aug 19, 2019 at 5:40 PM Igor Opaniuk <igor.opaniuk@gmail.com> wrote: > Hi Eugeniu, > > On Mon, Aug 19, 2019 at 11:28 AM Eugeniu Rosca <erosca@de.adit-jv.com> > wrote: > > > > Hi all, > > > > On Fri, Aug 16, 2019 at 11:46:03AM -0400, David Zeuthen wrote: > > > Hi, > > > As for upstreaming libavb patches, I'd be interested in landing them > > > upstream... makes it easier for anyone. > > > Our upstream is AOSP and we use gerrit for > > > code-review: > https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb > > > Here's a guide to > > > contributing: > https://source.android.com/setup/contribute/submit-patches ... > > > hope it's not too painful to use the AOSP process! > > > Thanks, > > > David > > > > David, thanks for being open about discussing/accepting the fixes. > > > > U-Boot folks, > > > > What's your opinion w.r.t. the right/best order between accepting > > the libavb fixes locally in U-Boot and pushing them to upstream? > > Which should come first? I suppose they can be first applied internally > > and replaced later on if they get reworked/optimized during upstreaming. > > > > -- > > Best Regards, > > Eugeniu. > > IMHO, the best approach here is to upstream fixes to AOSP libavb first, > only then port stuff to U-boot. > > Nevertheless, taking into account the amount of time needed > (sometimes it can take months, or even years, Sam has good > stories about that) to get something applied to AOSP, I would suggest > to proceed internally with commits like this ASAP, but somehow (extended > commit message for each "libavb sync" patch with the list of patches out of > AOSP tree; or introduce "doc/android/libavb-porting.txt", where we can > provide all details about list of commits from AOSP + out-of-tree commits, > how-to do porting etc; other ideas?) keep track of changes that are not > up-streamed to AOSP yet. > > Ideally we have to achieve 1:1 mapping (with the only difference - SPDX > tags), > of [1] in lib/libavb/, otherwise we will fight with constant code > divergence (obviously). > > [1] https://android.googlesource.com/platform/external/avb/+/ > <revision>/libavb > > -- > Best regards - Freundliche Grüsse - Meilleures salutations > > Igor Opaniuk > > mailto: igor.opaniuk@gmail.com > skype: igor.opanyuk > +380 (93) 836 40 67 > http://ua.linkedin.com/in/iopaniuk >
On Thu, Aug 15, 2019 at 11:04:03PM +0300, Sam Protsenko wrote: > After updating libavb to most recent version from AOSP/master, two new > warnings appear: > > Warning #1: > > lib/libavb/avb_cmdline.c: In function 'avb_append_options': > lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be > used uninitialized in this function > [-Wmaybe-uninitialized] > new_ret = avb_replace( > ^~~~~~~~~~~~ > slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used > uninitialized in this function > [-Wmaybe-uninitialized] > if (!cmdline_append_option( > ^~~~~~~~~~~~~~~~~~~~~~ > slot_data, "androidboot.veritymode", verity_mode)) { > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Warning #2: > > lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify': > lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used > uninitialized in this function > [-Wmaybe-uninitialized] > AvbSlotVerifyResult ret; > ^~~ > > Fix those by providing default return values to affected functions. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> Applied to u-boot/master, thanks!
diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c index cb5b98e423..684c512bb9 100644 --- a/lib/libavb/avb_cmdline.c +++ b/lib/libavb/avb_cmdline.c @@ -357,7 +357,8 @@ AvbSlotVerifyResult avb_append_options( // Should never get here because MANAGED_RESTART_AND_EIO is // remapped by avb_manage_hashtree_error_mode(). avb_assert_not_reached(); - break; + ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; + goto out; default: ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; goto out; diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c index 5d400b38aa..c0defdf9c9 100644 --- a/lib/libavb/avb_slot_verify.c +++ b/lib/libavb/avb_slot_verify.c @@ -1346,7 +1346,7 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops, AvbSlotVerifyFlags flags, AvbHashtreeErrorMode hashtree_error_mode, AvbSlotVerifyData** out_data) { - AvbSlotVerifyResult ret; + AvbSlotVerifyResult ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT; AvbSlotVerifyData* slot_data = NULL; AvbAlgorithmType algorithm_type = AVB_ALGORITHM_TYPE_NONE; bool using_boot_for_vbmeta = false;
After updating libavb to most recent version from AOSP/master, two new warnings appear: Warning #1: lib/libavb/avb_cmdline.c: In function 'avb_append_options': lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] new_ret = avb_replace( ^~~~~~~~~~~~ slot_data->cmdline, "$(ANDROID_VERITY_MODE)", dm_verity_mode); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used uninitialized in this function [-Wmaybe-uninitialized] if (!cmdline_append_option( ^~~~~~~~~~~~~~~~~~~~~~ slot_data, "androidboot.veritymode", verity_mode)) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Warning #2: lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify': lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used uninitialized in this function [-Wmaybe-uninitialized] AvbSlotVerifyResult ret; ^~~ Fix those by providing default return values to affected functions. Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> --- lib/libavb/avb_cmdline.c | 3 ++- lib/libavb/avb_slot_verify.c | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-)