boot_serial: Fix incorrect format specifier for off_t #1413
Conversation
|
FYI @nvlsianpu |
|
Hi, may I know how's the progress of this pull request? Is it pending for another reviewer before it can be merged? |
There was a problem hiding this comment.
I'm a little concerned if the 'j' is available in all of the printf variants that might be available to us. It doesn't look like nanolibc supports it, and I wonder if this will work in those configurations.
Intmax might also be fairly inefficient on some of these targets, but I'm not sure of a better way to handle it, at least portably.
| @@ -301,7 +301,8 @@ static off_t erase_range(const struct flash_area *fap, off_t start, off_t end) | |||
| } | |||
|
|
|||
| size = flash_sector_get_off(§) + flash_sector_get_size(§) - start; | |||
| BOOT_LOG_INF("Erasing range 0x%x:0x%x", start, start + size - 1); | |||
There was a problem hiding this comment.
Why not to cast this to ssize and use z?
There was a problem hiding this comment.
because ssize_t is not guaranteed to be the largest integer type supported and/or the same size as off_t.
There was a problem hiding this comment.
because
ssize_tis not guaranteed to be the largest integer type supported and/or the same size asoff_t.
We should probably replace the off_t, here, with the uint32_t, including the erase_range definition; I do not think that we need offsets and sizes greater than 32-bit here (for example Zephyr flash_map sizes are uint32_t). Than just use llx and cast expressions to unsigned long long - this should cover both 32 and 64 bit systems for us.
Scratch that, is should be possible to access slot anywhere on the external flash, so it means that it may be beyond 32-bit range, so off_t would be correct here, if we can make off_t 64bit on 32bit machines to make this work.
The Also, as far as Zephyr is concerned, it is supported by all toolchains/libc; but, as you noted, that might not be the case for the rest of the targets that mcuboot supports. If there indeed is a toolchain/libc supported by the mcuboot that does not support the |
|
Hi @stephanosio, in the previous comment you mention that you will revise the patch may I know how is the progress? |
That is "if there indeed is a toolchain/libc supported by the mcuboot that does not support the j modifier." Someone will need to approve the CI run here so we can see. |
|
@nvlsianpu @de-nordic @ioannisg @utzig @d3zd3z |
I do not see how the zephyrproject-rtos/zephyr#46443](zephyrproject-rtos/zephyr#46443 is depending on this. |
Because I did a workaround to just pass the workflow check for the flash_shell.c, so the reviewer can continue review on the other part, once this PR 1413 is checked in will remove the work around patch |
That's the reason why I keep rushing to complete this PR |
|
Hi @de-nordic, Is @nvlsianpu the maintainer? He seems like not very responsive. |
He's on holiday. |
I see, thanks for informing. Other than him, is there any other colleague that would be able to help? |
|
Hi @stephanosio, can you help to take a look at the CI result? |
|
Hi @stephanosio, any updates? |
This is not a problem specific to this PR. The mcuboot maintainers will need to fix their CI. p.s. the CI is tracking the Zephyr main repo, which is subject to breakages. This is generally a bad practice -- it should either track a specific release or commit. |
|
Hi, @utzig @d3zd3z @gustavonihei @nvlsianpu @de-nordic @ioannisg. Can you please take a look at the CI? |
|
@ngboonkhai @stephanosio Can you rebase? This looks like the "too big flash footprint" issue we recently fixed on the mian branch. |
Hi @nvlsianpu, Need stephan help to rebase, I'm here to keep track this issue to unblock my PR in Zephyr zephyrproject-rtos/zephyr#46443 |
The `BOOT_LOG_INF` function, which uses the format specifiers defined by the C standard, was incorrectly printing a variable with the type of `off_t` using the `%x` format specifier, which is intended to be used with the `int` type. The `off_t` type, specified by the POSIX standard, is not guaranteed to be `int`, and it may be defined as `long` or `long long` depending on the toolchain and the target architecture. This commit updates the print routine such that it casts the arguments of the `off_t` type to `intmax_t` and prints them out using the corresponding `%jx` format specifier. Signed-off-by: Stephanos Ioannidis <[email protected]>
stephanosio
force-pushed
the
fix_off_t_print_upstream
branch
from
2ce9f1e to
ddd8888
Compare
|
Rebased |
|
Thanks everyone for he effort. |
|
@ngboonkhai Yes I will do this on Monday. |
@nvlsianpu Okay, Thanks a lot :D |
|
Hi @nvlsianpu Quick check with you on the mcuboot fork sync, have you got a chance to look into it? I try my luck to search over the zephyr PR but can't find. |
|
Thanks @nvlsianpu |
|
Hi @nvlsianpu I believe you need to update the revision number at the zephyr west.yaml as well |
|
The
BOOT_LOG_INFfunction, which uses the format specifiers definedby the C standard, was incorrectly printing a variable with the type of
off_tusing the%xformat specifier, which is intended to be usedwith the
inttype.The
off_ttype, specified by the POSIX standard, is not guaranteed tobe
int, and it may be defined aslongorlong longdepending onthe toolchain and the target architecture.
This commit updates the print routine such that it casts the arguments
of the
off_ttype tointmax_tand prints them out using thecorresponding
%jxformat specifier.Signed-off-by: Stephanos Ioannidis [email protected]