Discussion:
[PATCH] udev: Allow acpi_index and index to be "0"
(too old to reply)
Joe Hershberger
2018-09-28 20:46:05 UTC
Permalink
0 can be a valid index returned by the BIOS, so allow that literal while
still checking for 0 as a failed conversion by strtoul(). Also, unsigned
long cannot be negative, so don't misleadingly check for less than 0.

Signed-off-by: Joe Hershberger <***@ni.com>
---
src/udev/udev-builtin-net_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c
index 5341d3788..338a2d4c4 100644
--- a/src/udev/udev-builtin-net_id.c
+++ b/src/udev/udev-builtin-net_id.c
@@ -232,7 +232,7 @@ static int dev_pci_onboard(struct udev_device *dev, struct netnames *names) {
return -ENOENT;

idx = strtoul(attr, NULL, 0);
- if (idx <= 0)
+ if (idx == 0 && strcmp("0", attr))
return -EINVAL;

/* Some BIOSes report rubbish indexes that are excessively high (2^24-1 is an index VMware likes to report for
--
2.11.0
systemd github import bot
2018-09-28 21:06:18 UTC
Permalink
Patchset imported to github.
To create a pull request, one of the main developers has to initiate one via:
<https://github.com/systemd/systemd/compare/master...systemd-mailing-devs:20180928204605.55321-1-joe.hershberger%40ni.com>

--
Generated by https://github.com/haraldh/mail2git
Mantas Mikulėnas
2018-09-29 10:15:37 UTC
Permalink
Post by Joe Hershberger
0 can be a valid index returned by the BIOS, so allow that literal while
still checking for 0 as a failed conversion by strtoul(). Also, unsigned
long cannot be negative, so don't misleadingly check for less than 0.
---
src/udev/udev-builtin-net_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/udev/udev-builtin-net_id.c
b/src/udev/udev-builtin-net_id.c
index 5341d3788..338a2d4c4 100644
--- a/src/udev/udev-builtin-net_id.c
+++ b/src/udev/udev-builtin-net_id.c
@@ -232,7 +232,7 @@ static int dev_pci_onboard(struct udev_device *dev,
struct netnames *names) {
return -ENOENT;
idx = strtoul(attr, NULL, 0);
- if (idx <= 0)
+ if (idx == 0 && strcmp("0", attr))
If error checking is needed, wouldn't it be better to use the error
checking that strtoul itself provides?

("If endptr is not NULL, strtoul() stores the address of the first invalid
character in *endptr.")
Post by Joe Hershberger
--
Mantas Mikulėnas
Lennart Poettering
2018-10-01 09:56:44 UTC
Permalink
Post by Joe Hershberger
0 can be a valid index returned by the BIOS, so allow that literal while
still checking for 0 as a failed conversion by strtoul(). Also, unsigned
long cannot be negative, so don't misleadingly check for less than 0.
---
src/udev/udev-builtin-net_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c
index 5341d3788..338a2d4c4 100644
--- a/src/udev/udev-builtin-net_id.c
+++ b/src/udev/udev-builtin-net_id.c
@@ -232,7 +232,7 @@ static int dev_pci_onboard(struct udev_device *dev, struct netnames *names) {
return -ENOENT;
idx = strtoul(attr, NULL, 0);
- if (idx <= 0)
+ if (idx == 0 && strcmp("0", attr))
return -EINVAL;
So, strtoul() error checking is messy. Let's just clean this up
properly: we have a call safe_atolu() in parse-util.h in place already
that we use preferably over raw strtoul() and that returns errors in a
more sane form. Hence, could you please rework your patch to that this
uses safe_atolu() instead of strtoul()? That would make it shorter,
cleaner and more elegant.

Also, could you please submit this as github PR?

https://github.com/systemd/systemd/pulls

Lennart
--
Lennart Poettering, Red Hat
Joe Hershberger
2018-10-01 20:52:44 UTC
Permalink
Hi Lennart,

On Mon, Oct 1, 2018 at 4:56 AM, Lennart Poettering
Post by Lennart Poettering
Post by Joe Hershberger
0 can be a valid index returned by the BIOS, so allow that literal while
still checking for 0 as a failed conversion by strtoul(). Also, unsigned
long cannot be negative, so don't misleadingly check for less than 0.
---
src/udev/udev-builtin-net_id.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c
index 5341d3788..338a2d4c4 100644
--- a/src/udev/udev-builtin-net_id.c
+++ b/src/udev/udev-builtin-net_id.c
@@ -232,7 +232,7 @@ static int dev_pci_onboard(struct udev_device *dev, struct netnames *names) {
return -ENOENT;
idx = strtoul(attr, NULL, 0);
- if (idx <= 0)
+ if (idx == 0 && strcmp("0", attr))
return -EINVAL;
So, strtoul() error checking is messy. Let's just clean this up
properly: we have a call safe_atolu() in parse-util.h in place already
that we use preferably over raw strtoul() and that returns errors in a
more sane form. Hence, could you please rework your patch to that this
uses safe_atolu() instead of strtoul()? That would make it shorter,
cleaner and more elegant.
OK, I changed the patch to use that instead.
Post by Lennart Poettering
Also, could you please submit this as github PR?
Done. I created a pull request instead of sending a patch email.

Thanks,
-Joe
Post by Lennart Poettering
https://github.com/systemd/systemd/pulls
Lennart
--
Lennart Poettering, Red Hat
_______________________________________________
systemd-devel mailing list
https://lists.freedesktop.org/mailman/listinfo/systemd-devel
Continue reading on narkive:
Loading...