udevadm: use parse_sec instead of atoi#4331
udevadm: use parse_sec instead of atoi#4331martinpitt merged 1 commit intosystemd:masterfrom stefan-it:udevadm-use-safe_atoi
Conversation
src/udev/udevadm-control.c
Outdated
| if (seconds >= 0) | ||
| timeout = seconds; | ||
| else | ||
| rc = safe_atoi(optarg, &seconds); |
There was a problem hiding this comment.
Hmm, I think if we rework this we should change this to use parse_sec() instead, which is a superset of atoi() in this context, and understands the usual "h", "min", ... suffixes. (of course, it actually returns usec_t, instead of int, but converting that should be simple enough...
There was a problem hiding this comment.
What do you think about the following code:
case 't': {
usec_t s;
int seconds;
int r;
r = parse_sec(optarg, &s);
if (r < 0) {
log_error("Failed to parse timeout value '%s'.", optarg);
return rc;
}
if ((s/USEC_PER_SEC) > INT_MAX)
log_error("Timeout value is out of range.");
else {
seconds = (int) (s/USEC_PER_SEC);
timeout = seconds;
rc = 0;
}
break;
}As the timeout variable is an int I added an overflow detection:
[root@arch-mse systemd]# ./udevadm control --timeout="infinity"
Timeout value is out of range.
[root@arch-mse systemd]# ./udevadm control --timeout="2147483648"
Timeout value is out of range.parse_sec calls parse_time and no "negative" values are allowed, so there's no need for an underflow detection, because in this cases a "Failed to parse timeout value" message will be returned:
[root@arch-mse systemd]# ./udevadm control --timeout="-5d"
Failed to parse timeout value '-5d'.In case of no error the return value 0 would be returned (before this patch, 1 was returned).
There was a problem hiding this comment.
the "return rc" should be "return r".
And I'd probably round up in the division... i.e.(s+USEC_PER_SEC-1)/USEC_PER_SEC, but I figure that doesn't matter much...
looks great otherwise...
There was a problem hiding this comment.
Then we could support the infinity option as --timeout value:
seconds = s != USEC_INFINITY ? (int) ((s + USEC_PER_SEC - 1) / USEC_PER_SEC) : INT_MAX;I'll replace the fprintf calls + push it again!
src/udev/udevadm-control.c
Outdated
| rc = safe_atoi(optarg, &seconds); | ||
|
|
||
| if (rc < 0 || seconds < 0) | ||
| fprintf(stderr, "invalid timeout value\n"); |
There was a problem hiding this comment.
For extra bonus points change this to log_error()...
poettering
added
udev
reviewed/needs-rework 🔨
stefan-it
changed the title
parse_sec instead of atoi
stefan-it
changed the title
parse_sec instead of atoi
|
I force pushed the latest changes, but one CI build fails with: checking that there are no running jobs
autopkgtest [20:17:48]: test boot-smoke: -----------------------]
autopkgtest [20:17:48]: test boot-smoke: - - - - - - - - - - results - - - - - - - - - -
boot-smoke PASS
autopkgtest [20:17:49]: @@@@@@@@@@@@@@@@@@@@ summary
timedated PASS
hostnamed PASS
localed-locale PASS
localed-x11-keymap PASS
logind PASS
unit-config PASS
storage PASS
networkd-test.py PASS
build-login PASS
boot-and-services PASS
udev PASS
upstream FAIL non-zero exit status 1
boot-smoke PASS
Exit request sent. |
|
CI failure looks unrelated. @martinpitt, any idea? |
src/udev/udevadm-control.c
Outdated
| if (seconds >= 0) | ||
| r = parse_sec(optarg, &s); | ||
| if (r < 0) { | ||
| log_error("Failed to parse timeout value '%s'.", optarg); |
There was a problem hiding this comment.
one more improvement: shorten this to:
if (r < 0)
return log_error_errno(r, "Failed to parse timeout value '%s'.", optarg);
|
looks great. one suggestion though, see above. might be worth fixing this and pushing again, maybe the CI when restarted that way cleans up itself on its own ;-) |
log_error method is used instead of fprintf
|
lgtm |
poettering
added
good-to-merge/waiting-for-ci 👍
|
Do you still have a pointer to the previous log that has the failed i386 test? |
|
Yes, it can be found here :) |
|
All tests green now, landing. Thanks! So in that failed run, |
I have already seen: #4319 (comment) |
We don't print all journal files. This is misleading a bit: systemd/systemd#4331 (comment) systemd/systemd#4395 (comment)
We don't print all journal files. This is misleading a bit: systemd/systemd#4331 (comment) systemd/systemd#4395 (comment)
Imported using git-ubuntu import. Changelog parent: 018d813 New changelog entries: [ Martin Pitt ] * debian/tests/unit-config: Query pkg-config for system unit dir. This fixes confusion on merged-/usr systems where both /usr/lib/systemd and /lib/systemd exist. It's actually useful to verify that systemd.pc says the truth. * debian/tests/upstream: Fix clobbering of merged-/usr symlinks * debian/tests/systemd-fsckd: Create /etc/default/grub.d if necessary * debian/rules: Drop check for linking to libs in /usr. This was just an approximation, as booting without an initrd could still be broken by library updates (e. g. #828991). With merged /usr now being the default this is now completely moot. * Move kernel-install initrd script to a later prefix. 60- does not leave much room for scripts that want to run before initrd building (which is usually one of the latest things to do), so bump to 85. Thanks to Sjoerd Simons for the suggestion. * Disable 99-default.link instead of the udev rule for disabling persistent interface names. Disabling 80-net-setup-link.rules will also cause ID_NET_DRIVER to not be set any more, which breaks 80-container-ve.network and matching on driver name in general. So disable the actual default link policy instead. Still keep testing for 80-net-setup-link.rules in the upgrade fix and 73-usb-net-by-mac.rules to keep the desired behaviour on systems which already disabled ifnames via that udev rule. See https://lists.freedesktop.org/archives/systemd-devel/2016-November/037805.html * debian/tests/boot-and-services: Always run seccomp test seccomp is now available on all architectures on which Debian and Ubuntu run tests, so stop making this test silently skip if seccomp is disabled. * Bump libseccomp build dependency as per configure.ac. * Replace "Drop RestrictAddressFamilies=" patch with sed call. With that it will also apply to upstream builds/CI, and it is structurally simpler. * Rebuild against libseccomp with fixed shlibs. (Closes: #844497) [ Michael Biebl ] * fstab-generator: add x-systemd.mount-timeout option. (Closes: #843989) * build-sys: do not install ctrl-alt-del.target symlink twice. (Closes: #844039) * Enable lz4 support. While the compression rate is not as good as XZ, it is much faster, so a better default for the journal and especially systemd-coredump. (Closes: #832010) [ Felipe Sateler ] * Enable machines.target by default. (Closes: #806787) [ Evgeny Vereshchagin ] * debian/tests/upstream: Print all journal files. We don't print all journal files. This is misleading a bit: systemd#4331 (comment) systemd#4395 (comment) [ Luca Boccassi ] * Use mount --move in initramfs-tools udev script. Due to recent changes in busybox and initramfs-tools the mount utility is no longer the one from busybox but from util-linux. The latter does not support mount -o move. The former supports both -o move and --move, so use it instead to be compatible with both. See this discussion for more details: https://bugs.debian.org/823856 (Closes: #844775)
4 participants
This PR uses the
safe_atoifunction for parsing the--timeoutcommand line argument forudevadm.