| Message ID | 20191130194240.10517-11-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
30.11.2019 22:42, Markus Armbruster wrote: > fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. > Except it doesn't when its @errp argument is &error_fatal or > &error_abort, because it blindly passes @errp to fit_image_addr(). > Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". > > The bug can't bite as no caller actually passes &error_fatal or > &error_abort. Fix it anyway. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Hmm, actually it makes my "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt" unnecessary. If you want you may drop my 01 (as it covers less problems), and in this case you may note in your cover letter, that errp = NULL is broken here too (may be nobady pass it?), and normal errp is handled wrong, as *errp doesn't set to NULL after error_free(*errp) (still, the only caller rely on return value more than on err, so the problem can't be triggered with current code) > --- > hw/core/loader-fit.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c > index 953b16bc82..c465921b8f 100644 > --- a/hw/core/loader-fit.c > +++ b/hw/core/loader-fit.c > @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > int cfg, void *opaque, const void *match_data, > hwaddr kernel_end, Error **errp) > { > + Error *err = NULL; > const char *name; > const void *data; > const void *load_data; > hwaddr load_addr; > - int img_off, err; > + int img_off; > size_t sz; > int ret; > > @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > return -EINVAL; > } > > - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); > - if (err == -ENOENT) { > + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); > + if (ret == -ENOENT) { > load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); > - error_free(*errp); > - } else if (err) { > - error_prepend(errp, "unable to read FDT load address from FIT: "); > - ret = err; > + error_free(err); > + } else if (ret) { > + error_propagate_prepend(errp, err, > + "unable to read FDT load address from FIT: "); > goto out; > } > > So much attention to that function :) I'd also propose the following: diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c index c465921b8f..2c9efeef7a 100644 --- a/hw/core/loader-fit.c +++ b/hw/core/loader-fit.c @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, { Error *err = NULL; const char *name; - const void *data; - const void *load_data; + void *data; + void *load_data; hwaddr load_addr; int img_off; size_t sz; @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, ret = 0; out: - g_free((void *) data); + g_free(data); if (data != load_data) { - g_free((void *) load_data); + g_free(load_data); } return ret; } Or, even better: --- a/hw/core/loader-fit.c +++ b/hw/core/loader-fit.c @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, { Error *err = NULL; const char *name; - const void *data; + g_autofree void *data = NULL; + g_autofree void *fdt_filter_data = NULL; const void *load_data; hwaddr load_addr; int img_off; @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, if (ret == -ENOENT) { load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); error_free(err); + return 0; } else if (ret) { error_propagate_prepend(errp, err, "unable to read FDT load address from FIT: "); - goto out; + return ret; } if (ldr->fdt_filter) { - load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr); + load_data = fdt_filter_data = + ldr->fdt_filter(opaque, data, match_data, &load_addr); } load_addr = ldr->addr_to_phys(opaque, load_addr); sz = fdt_totalsize(load_data); rom_add_blob_fixed(name, load_data, sz, load_addr); - ret = 0; -out: - g_free((void *) data); - if (data != load_data) { - g_free((void *) load_data); - } - return ret; + return 0; }
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 30.11.2019 22:42, Markus Armbruster wrote: >> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. >> Except it doesn't when its @errp argument is &error_fatal or >> &error_abort, because it blindly passes @errp to fit_image_addr(). >> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". >> >> The bug can't bite as no caller actually passes &error_fatal or >> &error_abort. Fix it anyway. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > Hmm, actually it makes my > "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt" > unnecessary. If you want you may drop my 01 (as it covers less problems), Yes. > and in this case you may note in your cover letter, that > errp = NULL is broken here too (may be nobady pass it?), You're right, null @errp is wrong, too. > and normal errp is handled wrong, as *errp doesn't set to NULL after > error_free(*errp) Yes, that's wrong, too. fit_load_fdt() itself doesn't use *errp after freeing it, but it sets a trap for its callers. > (still, the only caller rely on return value more than on > err, so the problem can't be triggered with current code) True. New commit message (based on v2's): hw/core: Fix fit_load_fdt() error API violations fit_load_fdt() passes @errp to fit_image_addr(), then recovers from ENOENT failures. Passing @errp is wrong, because it works only as long as @errp is neither @error_fatal nor @error_abort. Error recovery dereferences @errp. That's also wrong; see the big comment in error.h. Error recovery can leave *errp pointing to a freed Error object. Wrong, it must be null on success. Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". No caller actually passes such values, or uses *errp on success. Fix anyway: splice in a local Error *err, and error_propagate(). Signed-off-by: Markus Armbruster <armbru@redhat.com> Okay? > >> --- >> hw/core/loader-fit.c | 15 ++++++++------- >> 1 file changed, 8 insertions(+), 7 deletions(-) >> >> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c >> index 953b16bc82..c465921b8f 100644 >> --- a/hw/core/loader-fit.c >> +++ b/hw/core/loader-fit.c >> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> int cfg, void *opaque, const void *match_data, >> hwaddr kernel_end, Error **errp) >> { >> + Error *err = NULL; >> const char *name; >> const void *data; >> const void *load_data; >> hwaddr load_addr; >> - int img_off, err; >> + int img_off; >> size_t sz; >> int ret; >> >> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> return -EINVAL; >> } >> >> - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); >> - if (err == -ENOENT) { >> + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); >> + if (ret == -ENOENT) { >> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); >> - error_free(*errp); >> - } else if (err) { >> - error_prepend(errp, "unable to read FDT load address from FIT: "); >> - ret = err; >> + error_free(err); >> + } else if (ret) { >> + error_propagate_prepend(errp, err, >> + "unable to read FDT load address from FIT: "); >> goto out; >> } >> >> > > So much attention to that function :) > > I'd also propose the following: > > diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c > index c465921b8f..2c9efeef7a 100644 > --- a/hw/core/loader-fit.c > +++ b/hw/core/loader-fit.c > @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > { > Error *err = NULL; > const char *name; > - const void *data; > - const void *load_data; > + void *data; > + void *load_data; > hwaddr load_addr; > int img_off; > size_t sz; > @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > > ret = 0; > out: > - g_free((void *) data); > + g_free(data); > if (data != load_data) { > - g_free((void *) load_data); > + g_free(load_data); > } > return ret; > } > > > Or, even better: > > --- a/hw/core/loader-fit.c > +++ b/hw/core/loader-fit.c > @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > { > Error *err = NULL; > const char *name; > - const void *data; > + g_autofree void *data = NULL; > + g_autofree void *fdt_filter_data = NULL; > const void *load_data; > hwaddr load_addr; > int img_off; > @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, > if (ret == -ENOENT) { > load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); > error_free(err); > + return 0; > } else if (ret) { > error_propagate_prepend(errp, err, > "unable to read FDT load address from FIT: "); > - goto out; > + return ret; > } > > if (ldr->fdt_filter) { > - load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr); > + load_data = fdt_filter_data = > + ldr->fdt_filter(opaque, data, match_data, &load_addr); > } > > load_addr = ldr->addr_to_phys(opaque, load_addr); > sz = fdt_totalsize(load_data); > rom_add_blob_fixed(name, load_data, sz, load_addr); > > - ret = 0; > -out: > - g_free((void *) data); > - if (data != load_data) { > - g_free((void *) load_data); > - } > - return ret; > + return 0; > } Looks like a sensible separate cleanup patch to me. Care to post it? Thanks!
06.12.2019 10:46, Markus Armbruster wrote: > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > >> 30.11.2019 22:42, Markus Armbruster wrote: >>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. >>> Except it doesn't when its @errp argument is &error_fatal or >>> &error_abort, because it blindly passes @errp to fit_image_addr(). >>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". >>> >>> The bug can't bite as no caller actually passes &error_fatal or >>> &error_abort. Fix it anyway. >>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> >> Hmm, actually it makes my >> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt" >> unnecessary. If you want you may drop my 01 (as it covers less problems), > > Yes. > >> and in this case you may note in your cover letter, that >> errp = NULL is broken here too (may be nobady pass it?), > > You're right, null @errp is wrong, too. > >> and normal errp is handled wrong, as *errp doesn't set to NULL after >> error_free(*errp) > > Yes, that's wrong, too. fit_load_fdt() itself doesn't use *errp after > freeing it, but it sets a trap for its callers. > >> (still, the only caller rely on return value more than on >> err, so the problem can't be triggered with current code) > > True. > > New commit message (based on v2's): > > hw/core: Fix fit_load_fdt() error API violations > > fit_load_fdt() passes @errp to fit_image_addr(), then recovers from > ENOENT failures. Passing @errp is wrong, because it works only as > long as @errp is neither @error_fatal nor @error_abort. Error > recovery dereferences @errp. That's also wrong; see the big comment > in error.h. Error recovery can leave *errp pointing to a freed > Error object. Wrong, it must be null on success. Messed up in > commit 3eb99edb48 "loader-fit: Wean off error_printf()". > > No caller actually passes such values, or uses *errp on success. > > Fix anyway: splice in a local Error *err, and error_propagate(). > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > > Okay? Yes) also add Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > >> >>> --- >>> hw/core/loader-fit.c | 15 ++++++++------- >>> 1 file changed, 8 insertions(+), 7 deletions(-) >>> >>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c >>> index 953b16bc82..c465921b8f 100644 >>> --- a/hw/core/loader-fit.c >>> +++ b/hw/core/loader-fit.c >>> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> int cfg, void *opaque, const void *match_data, >>> hwaddr kernel_end, Error **errp) >>> { >>> + Error *err = NULL; >>> const char *name; >>> const void *data; >>> const void *load_data; >>> hwaddr load_addr; >>> - int img_off, err; >>> + int img_off; >>> size_t sz; >>> int ret; >>> >>> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> return -EINVAL; >>> } >>> >>> - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); >>> - if (err == -ENOENT) { >>> + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); >>> + if (ret == -ENOENT) { >>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); >>> - error_free(*errp); >>> - } else if (err) { >>> - error_prepend(errp, "unable to read FDT load address from FIT: "); >>> - ret = err; >>> + error_free(err); >>> + } else if (ret) { >>> + error_propagate_prepend(errp, err, >>> + "unable to read FDT load address from FIT: "); >>> goto out; >>> } >>> >>> >> >> So much attention to that function :) >> >> I'd also propose the following: >> >> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c >> index c465921b8f..2c9efeef7a 100644 >> --- a/hw/core/loader-fit.c >> +++ b/hw/core/loader-fit.c >> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> { >> Error *err = NULL; >> const char *name; >> - const void *data; >> - const void *load_data; >> + void *data; >> + void *load_data; >> hwaddr load_addr; >> int img_off; >> size_t sz; >> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> >> ret = 0; >> out: >> - g_free((void *) data); >> + g_free(data); >> if (data != load_data) { >> - g_free((void *) load_data); >> + g_free(load_data); >> } >> return ret; >> } >> >> >> Or, even better: >> >> --- a/hw/core/loader-fit.c >> +++ b/hw/core/loader-fit.c >> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> { >> Error *err = NULL; >> const char *name; >> - const void *data; >> + g_autofree void *data = NULL; >> + g_autofree void *fdt_filter_data = NULL; >> const void *load_data; >> hwaddr load_addr; >> int img_off; >> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >> if (ret == -ENOENT) { >> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); >> error_free(err); >> + return 0; >> } else if (ret) { >> error_propagate_prepend(errp, err, >> "unable to read FDT load address from FIT: "); >> - goto out; >> + return ret; >> } >> >> if (ldr->fdt_filter) { >> - load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr); >> + load_data = fdt_filter_data = >> + ldr->fdt_filter(opaque, data, match_data, &load_addr); >> } >> >> load_addr = ldr->addr_to_phys(opaque, load_addr); >> sz = fdt_totalsize(load_data); >> rom_add_blob_fixed(name, load_data, sz, load_addr); >> >> - ret = 0; >> -out: >> - g_free((void *) data); >> - if (data != load_data) { >> - g_free((void *) load_data); >> - } >> - return ret; >> + return 0; >> } > > Looks like a sensible separate cleanup patch to me. Care to post it? > Yes, I'll send
06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote: > 06.12.2019 10:46, Markus Armbruster wrote: >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: >> >>> 30.11.2019 22:42, Markus Armbruster wrote: >>>> fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. >>>> Except it doesn't when its @errp argument is &error_fatal or >>>> &error_abort, because it blindly passes @errp to fit_image_addr(). >>>> Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". >>>> >>>> The bug can't bite as no caller actually passes &error_fatal or >>>> &error_abort. Fix it anyway. >>>> >>>> Signed-off-by: Markus Armbruster <armbru@redhat.com> >>> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> >>> Hmm, actually it makes my >>> "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt" >>> unnecessary. If you want you may drop my 01 (as it covers less problems), >> >> Yes. >> >>> and in this case you may note in your cover letter, that >>> errp = NULL is broken here too (may be nobady pass it?), >> >> You're right, null @errp is wrong, too. >> >>> and normal errp is handled wrong, as *errp doesn't set to NULL after >>> error_free(*errp) >> >> Yes, that's wrong, too. fit_load_fdt() itself doesn't use *errp after >> freeing it, but it sets a trap for its callers. >> >>> (still, the only caller rely on return value more than on >>> err, so the problem can't be triggered with current code) >> >> True. >> >> New commit message (based on v2's): >> >> hw/core: Fix fit_load_fdt() error API violations >> >> fit_load_fdt() passes @errp to fit_image_addr(), then recovers from >> ENOENT failures. Passing @errp is wrong, because it works only as >> long as @errp is neither @error_fatal nor @error_abort. Error >> recovery dereferences @errp. That's also wrong; see the big comment >> in error.h. Error recovery can leave *errp pointing to a freed >> Error object. Wrong, it must be null on success. Messed up in >> commit 3eb99edb48 "loader-fit: Wean off error_printf()". >> >> No caller actually passes such values, or uses *errp on success. >> >> Fix anyway: splice in a local Error *err, and error_propagate(). >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> >> Okay? > > Yes) also add > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > >> >>> >>>> --- >>>> hw/core/loader-fit.c | 15 ++++++++------- >>>> 1 file changed, 8 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c >>>> index 953b16bc82..c465921b8f 100644 >>>> --- a/hw/core/loader-fit.c >>>> +++ b/hw/core/loader-fit.c >>>> @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>>> int cfg, void *opaque, const void *match_data, >>>> hwaddr kernel_end, Error **errp) >>>> { >>>> + Error *err = NULL; >>>> const char *name; >>>> const void *data; >>>> const void *load_data; >>>> hwaddr load_addr; >>>> - int img_off, err; >>>> + int img_off; >>>> size_t sz; >>>> int ret; >>>> @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>>> return -EINVAL; >>>> } >>>> - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); >>>> - if (err == -ENOENT) { >>>> + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); >>>> + if (ret == -ENOENT) { >>>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); >>>> - error_free(*errp); >>>> - } else if (err) { >>>> - error_prepend(errp, "unable to read FDT load address from FIT: "); >>>> - ret = err; >>>> + error_free(err); >>>> + } else if (ret) { >>>> + error_propagate_prepend(errp, err, >>>> + "unable to read FDT load address from FIT: "); >>>> goto out; >>>> } >>>> >>> >>> So much attention to that function :) >>> >>> I'd also propose the following: >>> >>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c >>> index c465921b8f..2c9efeef7a 100644 >>> --- a/hw/core/loader-fit.c >>> +++ b/hw/core/loader-fit.c >>> @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> { >>> Error *err = NULL; >>> const char *name; >>> - const void *data; >>> - const void *load_data; >>> + void *data; >>> + void *load_data; >>> hwaddr load_addr; >>> int img_off; >>> size_t sz; >>> @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> >>> ret = 0; >>> out: >>> - g_free((void *) data); >>> + g_free(data); >>> if (data != load_data) { >>> - g_free((void *) load_data); >>> + g_free(load_data); >>> } >>> return ret; >>> } >>> >>> >>> Or, even better: >>> >>> --- a/hw/core/loader-fit.c >>> +++ b/hw/core/loader-fit.c >>> @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> { >>> Error *err = NULL; >>> const char *name; >>> - const void *data; >>> + g_autofree void *data = NULL; >>> + g_autofree void *fdt_filter_data = NULL; >>> const void *load_data; >>> hwaddr load_addr; >>> int img_off; >>> @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, >>> if (ret == -ENOENT) { >>> load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); >>> error_free(err); >>> + return 0; >>> } else if (ret) { >>> error_propagate_prepend(errp, err, >>> "unable to read FDT load address from FIT: "); >>> - goto out; >>> + return ret; >>> } >>> >>> if (ldr->fdt_filter) { >>> - load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr); >>> + load_data = fdt_filter_data = >>> + ldr->fdt_filter(opaque, data, match_data, &load_addr); >>> } >>> >>> load_addr = ldr->addr_to_phys(opaque, load_addr); >>> sz = fdt_totalsize(load_data); >>> rom_add_blob_fixed(name, load_data, sz, load_addr); >>> >>> - ret = 0; >>> -out: >>> - g_free((void *) data); >>> - if (data != load_data) { >>> - g_free((void *) load_data); >>> - } >>> - return ret; >>> + return 0; >>> } >> >> Looks like a sensible separate cleanup patch to me. Care to post it? >> > > Yes, I'll send > Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer.. So, I just don't understand the logic of the code (for me it's strange to free pointer, returned by function, returning const pointer) And what are the reasons, to make return type of fit_load_image_alloc constant, I don't see.. IMHO, there are no reasons. Same thing with .fdt_filter ... I don't want to dig in, so, sorry, I'll not send :)
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: > 06.12.2019 13:53, Vladimir Sementsov-Ogievskiy wrote: >> 06.12.2019 10:46, Markus Armbruster wrote: >>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes: [...] >>>> So much attention to that function :) >>>> >>>> I'd also propose the following: [...] >>> >>> Looks like a sensible separate cleanup patch to me. Care to post it? >>> >> >> Yes, I'll send >> > > > Hm, it doesn't compile, as fit_load_image_alloc return type is const pointer.. > So, I just don't understand the logic of the code (for me it's strange to > free pointer, returned by function, returning const pointer) > > And what are the reasons, to make return type of fit_load_image_alloc constant, > I don't see.. IMHO, there are no reasons. > Same thing with .fdt_filter ... > > I don't want to dig in, so, sorry, I'll not send :) That's okay.
diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c index 953b16bc82..c465921b8f 100644 --- a/hw/core/loader-fit.c +++ b/hw/core/loader-fit.c @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, int cfg, void *opaque, const void *match_data, hwaddr kernel_end, Error **errp) { + Error *err = NULL; const char *name; const void *data; const void *load_data; hwaddr load_addr; - int img_off, err; + int img_off; size_t sz; int ret; @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, return -EINVAL; } - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); - if (err == -ENOENT) { + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); + if (ret == -ENOENT) { load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); - error_free(*errp); - } else if (err) { - error_prepend(errp, "unable to read FDT load address from FIT: "); - ret = err; + error_free(err); + } else if (ret) { + error_propagate_prepend(errp, err, + "unable to read FDT load address from FIT: "); goto out; }
fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. Except it doesn't when its @errp argument is &error_fatal or &error_abort, because it blindly passes @errp to fit_image_addr(). Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". The bug can't bite as no caller actually passes &error_fatal or &error_abort. Fix it anyway. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- hw/core/loader-fit.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)