| Message ID | 82FD4A5824725B4AA82D85AB7C6F62E81C251E1E@gls-exch1.cryptera.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ubivol: Fix ubivolume-swap masking error from ubi update | expand |
Hi Mikael, On 23.06.20 16:08, Mikael Jepsen wrote: > When "replaces" property is set on ubi volumes, the > volumes are swapped even if ubi_update_start failed > (e.g. bad sha256). > This description does not match what I see in patch (or I have not understood the context). The description refers to swa volumes, but you are changing inside the UBI updater. Let's try to define first the issue. > This results in not only an unintended swap, but > also that swupdate incorrectly returns SUCCESS as > the original error is masked by the successfull swap > operation. I will better understand why it happens: > > Signed-off-by: Mikael Jepsen <mje@cryptera.com> > --- > handlers/ubivol_handler.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c > index 008e9f7..ae6d0b0 100644 > --- a/handlers/ubivol_handler.c > +++ b/handlers/ubivol_handler.c > @@ -203,7 +203,8 @@ static int update_volume(libubi_t libubi, struct img_type *img, > err = ubi_update_start(libubi, fdout, bytes); > if (err) { > ERROR("cannot start volume \"%s\" update", node); > - return -1; I agree that here a close(fdout) is missing, but the handler returns with error (-1) and SWUpdate should report failure. > + err = -1; Which is the meaning to set err ? > + goto update_volume_out; > } > > snprintf(sbuf, sizeof(sbuf), "Installing image %s into volume %s(%s)", > @@ -215,6 +216,7 @@ static int update_volume(libubi_t libubi, struct img_type *img, > if (copyimage(&fdout, img, NULL) < 0) { > ERROR("Error copying extracted file"); > err = -1; > + goto update_volume_out; > } > > /* handle replace */ > @@ -225,6 +227,7 @@ static int update_volume(libubi_t libubi, struct img_type *img, > vol->name, repl_vol->name, err); > } > > +update_volume_out: > close(fdout); > return err; > } > Can you better explain the issue and how this is related to the description in commit ? Best regards, Stefano Babic
Hi Stefano, Ahh, I now see that I messed up in my many re-wordings of the commit message. I should have stuck with my original "ubi update" wording and not replaced it with the explicit reference to ubi_update_start, which is incorrect. The error code being masked is the one coming from copyimage, not ubi_update_start. Sorry about that. I will updated the wording in an updated patch. I hope this better explains the problem and the patch contents? -----Original Message----- > From: Stefano Babic [mailto:sbabic@denx.de] > Sent: 23. juni 2020 18:41 > To: Mikael Jepsen; swupdate@googlegroups.com > Subject: Re: [swupdate] [PATCH] ubivol: Fix ubivolume-swap masking error > from ubi update > > Hi Mikael, > > On 23.06.20 16:08, Mikael Jepsen wrote: > > When "replaces" property is set on ubi volumes, the > > volumes are swapped even if ubi_update_start failed > > (e.g. bad sha256). > > > > This description does not match what I see in patch (or I have not > understood the context). > > The description refers to swa volumes, but you are changing inside the > UBI updater. Let's try to define first the issue. > > > This results in not only an unintended swap, but > > also that swupdate incorrectly returns SUCCESS as > > the original error is masked by the successfull swap > > operation. > > I will better understand why it happens: > > > > > Signed-off-by: Mikael Jepsen <mje@cryptera.com> > > --- > > handlers/ubivol_handler.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c > > index 008e9f7..ae6d0b0 100644 > > --- a/handlers/ubivol_handler.c > > +++ b/handlers/ubivol_handler.c > > @@ -203,7 +203,8 @@ static int update_volume(libubi_t libubi, struct > img_type *img, > > err = ubi_update_start(libubi, fdout, bytes); > > if (err) { > > ERROR("cannot start volume \"%s\" update", node); > > - return -1; > > I agree that here a close(fdout) is missing, but the handler returns > with error (-1) and SWUpdate should report failure. > > > + err = -1; > > Which is the meaning to set err ? In the update_volume function, there is a mix of returning immediately or setting err and returning it later, so I was not sure which you preferred. I opted to add the cleanup macro, a pattern I saw in other files, but I could also just close fdout and return -1 immediately and remove the cleanup macro if that is preferred? > > > + goto update_volume_out; > > } > > > > snprintf(sbuf, sizeof(sbuf), "Installing image %s into volume %s(%s)", > > @@ -215,6 +216,7 @@ static int update_volume(libubi_t libubi, struct > img_type *img, > > if (copyimage(&fdout, img, NULL) < 0) { > > ERROR("Error copying extracted file"); > > err = -1; > > + goto update_volume_out; > > } > > > > /* handle replace */ > > @@ -225,6 +227,7 @@ static int update_volume(libubi_t libubi, struct > img_type *img, > > vol->name, repl_vol->name, err); > > } > > > > +update_volume_out: > > close(fdout); > > return err; > > } > > > > Can you better explain the issue and how this is related to the > description in commit ? > > Best regards, > Stefano Babic > > > -- > ========================================================== > =========== > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de > ========================================================== > ===========
diff --git a/handlers/ubivol_handler.c b/handlers/ubivol_handler.c index 008e9f7..ae6d0b0 100644 --- a/handlers/ubivol_handler.c +++ b/handlers/ubivol_handler.c @@ -203,7 +203,8 @@ static int update_volume(libubi_t libubi, struct img_type *img, err = ubi_update_start(libubi, fdout, bytes); if (err) { ERROR("cannot start volume \"%s\" update", node); - return -1; + err = -1; + goto update_volume_out; } snprintf(sbuf, sizeof(sbuf), "Installing image %s into volume %s(%s)", @@ -215,6 +216,7 @@ static int update_volume(libubi_t libubi, struct img_type *img, if (copyimage(&fdout, img, NULL) < 0) { ERROR("Error copying extracted file"); err = -1; + goto update_volume_out; } /* handle replace */ @@ -225,6 +227,7 @@ static int update_volume(libubi_t libubi, struct img_type *img, vol->name, repl_vol->name, err); } +update_volume_out: close(fdout); return err; }
When "replaces" property is set on ubi volumes, the volumes are swapped even if ubi_update_start failed (e.g. bad sha256). This results in not only an unintended swap, but also that swupdate incorrectly returns SUCCESS as the original error is masked by the successfull swap operation. Signed-off-by: Mikael Jepsen <mje@cryptera.com> --- handlers/ubivol_handler.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)