[PATCH] BUG: copyhandler: fix discarded size on block device source
dit.k...@kynetics.com
When copyfrom points to a block device and no "size" property is
set in sw-description, copy_single_file() tries to auto-detect the
source size via ioctl(BLKGETSIZE64). The detection is coded as:
else if (S_ISBLK(statbuf.st_mode) &&
(ioctl(fdin, BLKGETSIZE64, &size) < 0)) {
ERROR("Cannot get size of Block Device %s", path);
size = -1;
}
else {
size = -1;
}
The ioctl() call only takes effect if the whole condition is true,
which requires the ioctl itself to fail (< 0). When the ioctl
succeeds, the combined condition evaluates to false, the branch is
skipped, and execution falls through to the generic "else" clause,
which overwrites the just-detected, valid size with -1.
As a result, size auto-detection for a block device source always
fails with "Size cannot be detected for %s", even when the ioctl
call itself works correctly and returns the right value.
Signed-off-by: Dit Kozmaj <dit.k...@kynetics.com>
---
handlers/copy_handler.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/handlers/copy_handler.c b/handlers/copy_handler.c
index bd31757d..f5826ad6 100644
--- a/handlers/copy_handler.c
+++ b/handlers/copy_handler.c
@@ -104,9 +104,11 @@ static int copy_single_file(const char *path, off_t skipbytes, ssize_t size, str
if (S_ISREG(statbuf.st_mode)) {
size = statbuf.st_size;
}
- else if (S_ISBLK(statbuf.st_mode) && (ioctl(fdin, BLKGETSIZE64, &size) < 0)) {
- ERROR("Cannot get size of Block Device %s", path);
- size = -1;
+ else if (S_ISBLK(statbuf.st_mode)) {
+ if (ioctl(fdin, BLKGETSIZE64, &size) < 0) {
+ ERROR("Cannot get size of Block Device %s", path);
+ size = -1;
+ }
}
#ifdef CONFIG_MTD
else if (S_ISCHR(statbuf.st_mode)) {
--
2.55.0
Stefano Babic
probably the answer is simple but I do not get the point.
On 7/9/26 14:23, dit.k...@kynetics.com wrote:
> From: Dit Kozmaj <dit.k...@kynetics.com>
>
> When copyfrom points to a block device and no "size" property is
> set in sw-description, copy_single_file() tries to auto-detect the
> source size via ioctl(BLKGETSIZE64). The detection is coded as:
>
> else if (S_ISBLK(statbuf.st_mode) &&
> (ioctl(fdin, BLKGETSIZE64, &size) < 0)) {
> ERROR("Cannot get size of Block Device %s", path);
> size = -1;
> }
> else {
> size = -1;
> }
>
> The ioctl() call only takes effect if the whole condition is true,
> which requires the ioctl itself to fail (< 0). When the ioctl
> succeeds, the combined condition evaluates to false, the branch is
> skipped, and execution falls through to the generic "else" clause,
> which overwrites the just-detected, valid size with -1.
first part of the expression fails, that means S_ISBLK() fails.
According to compiler (and C language), the second part of the
expression is not called when the first part fails, and it is not
possible that ioctl is successful because it doesn't run.
Nevertheless, S_ISBLK() is a check of consistency because ioctl(fdin,
BLKGETSIZE64, &size) can be called only if the device is a block device.
How can it be that ioctl() is successful when S_IBLK() fails ?
>
> As a result, size auto-detection for a block device source always
> fails with "Size cannot be detected for %s", even when the ioctl
> call itself works correctly and returns the right value.
>
> Signed-off-by: Dit Kozmaj <dit.k...@kynetics.com>
> ---
> handlers/copy_handler.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/handlers/copy_handler.c b/handlers/copy_handler.c
> index bd31757d..f5826ad6 100644
> --- a/handlers/copy_handler.c
> +++ b/handlers/copy_handler.c
> @@ -104,9 +104,11 @@ static int copy_single_file(const char *path, off_t skipbytes, ssize_t size, str
> if (S_ISREG(statbuf.st_mode)) {
> size = statbuf.st_size;
> }
> - else if (S_ISBLK(statbuf.st_mode) && (ioctl(fdin, BLKGETSIZE64, &size) < 0)) {
> - ERROR("Cannot get size of Block Device %s", path);
> - size = -1;
> + else if (S_ISBLK(statbuf.st_mode)) {
> + if (ioctl(fdin, BLKGETSIZE64, &size) < 0) {
shouldn't be a difference.
Can you help me to understand ?
Best regards,
Stefano Babic
> + ERROR("Cannot get size of Block Device %s", path);
> + size = -1;
> + }
> }
> #ifdef CONFIG_MTD
> else if (S_ISCHR(statbuf.st_mode)) {
--
Nabla Software Engineering GmbH
Hirschstr. 111A | 86156 Augsburg | Tel: +49 821 45592596
Geschäftsführer : Stefano Babic | HRB 40522 Augsburg
E-Mail: sba...@nabladev.com
Dit Kozmaj
Thanks for looking at this — the confusion is understandable, and I think it comes down to one specific point about short-circuit evaluation, so let me isolate it precisely.
You're right that && short-circuits: when the left operand is false, the right operand is never evaluated.
But that's not the situation here. In our case, S_ISBLK(statbuf.st_mode) is true — the source really is a block device (a real partition node).
Short-circuiting only skips evaluation when the left side is false; when the left side is true, C is required to evaluate the right side too, and the result of the whole && expression is simply whatever that right side evaluates to.
So walking through it concretely with S_ISBLK == true:
The ioctl() call does run (it's not skipped — the left side was true), and it does succeed, writing the correct size into size as a side effect.
But the comparison 0 < 0 is false, so the whole && expression is false — not because S_ISBLK failed, but because the ioctl succeeded.
The branch condition was deliberately written to only be true when the ioctl fails, so a successful ioctl makes the branch condition false and the code falls through to the trailing else, which unconditionally sets size = -1 and discarding the value the ioctl just correctly wrote.
So the one place that would keep a successful detection is unreachable — every successful ioctl call on a block device source falls straight through to the generic else { size = -1; }.
The fix simply separates "is this a block device" from "did the ioctl on it fail", so a successful call is no longer bound to a condition that requires failure to be true:
else if (S_ISBLK(statbuf.st_mode)) {
if (ioctl(fdin, BLKGETSIZE64, &size) < 0) {
ERROR("Cannot get size of Block Device %s", path);
size = -1;
}
/* else: size already holds the correct value from ioctl() */
}...
Happy to resend the patch with a clearer commit message if that would help reviewers avoid the same misreading — let me know.
Best regards,
Dit
Stefano Babic
On 7/10/26 17:29, Dit Kozmaj wrote:
>
> Thanks for looking at this — the confusion is understandable, and I
> think it comes down to one specific point about short-circuit
> evaluation, so let me isolate it precisely.
>
> But that's not the situation here. In our case, S_ISBLK(statbuf.st_mode)
> Short-circuiting only skips evaluation when the /left/ side is false;
> too, and the result of the whole && expression is simply whatever that
> right side evaluates to.
>
> So walking through it concretely with S_ISBLK == true:
>
> && ( ioctl() runs, returns 0 (success) < 0 ) true && false = false*
>
> The ioctl() call /does/ run (it's not skipped — the left side was true),
> and it /does/ succeed, writing the correct size into size as a side effect.
> But the comparison *0 < 0* is false, so the whole && expression is false
> — *not because S_ISBLK failed, but because the ioctl succeeded*.
> sets size = -1 and discarding the value the ioctl just correctly wrote.
>
> through to the generic else { size = -1; }.
> The fix simply separates "is this a block device" from "did the ioctl on
> it fail", so a successful call is no longer bound to a condition that
> requires failure to be true:
>
Best regards,
Stefano Babic
> You received this message because you are subscribed to the Google
> Groups "swupdate" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to swupdate+u...@googlegroups.com
> <mailto:swupdate+u...@googlegroups.com>.
> To view this discussion visit https://groups.google.com/d/msgid/
> swupdate/e6cafc0a-6ecf-458d-b3c1-decefd281012n%40googlegroups.com
> <https://groups.google.com/d/msgid/swupdate/e6cafc0a-6ecf-458d-b3c1-
> decefd281012n%40googlegroups.com?utm_medium=email&utm_source=footer>.