Re: [systemd-devel] [PATCH] udev: Allow acpi_index and index to be "0"

[Joe Hershberger]

Hi Lennart,

On Mon, Oct 1, 2018 at 4:56 AM, Lennart Poettering
 wrote:
> On Fr, 28.09.18 15:46, Joe Hershberger (joe.hershber...@ni.com) wrote:
>
>> 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 
>> ---
>>  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.

> Also, could you please submit this as github PR?

Done. I created a pull request instead of sending a patch email.

Thanks,
-Joe

> https://github.com/systemd/systemd/pulls
>
> Lennart
>
> --
> Lennart Poettering, Red Hat
> ___
> systemd-devel mailing list
> systemd-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/systemd-devel
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] udev: Allow acpi_index and index to be "0"

[Lennart Poettering]

On Fr, 28.09.18 15:46, Joe Hershberger (joe.hershber...@ni.com) wrote:

> 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 
> ---
>  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
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] udev: Allow acpi_index and index to be "0"

[Mantas Mikulėnas]

On Fri, Sep 28, 2018, 23:49 Joe Hershberger  wrote:

> 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 
> ---
>  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.")

>
> --

Mantas Mikulėnas
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] udev: Allow acpi_index and index to be "0"

[systemd github import bot]

Patchset imported to github.
To create a pull request, one of the main developers has to initiate one via:


--
Generated by https://github.com/haraldh/mail2git
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] udev: Allow acpi_index and index to be "0"

[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.

Signed-off-by: Joe Hershberger 
---
 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-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel