Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1461474 > unrolled thread
| Started by | Darren Hart <dvhart@infradead.org> |
|---|---|
| First post | 2016-08-12 23:50 +0200 |
| Last post | 2016-08-17 20:30 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH] asus-laptop: get rid of parse_arg() Darren Hart <dvhart@infradead.org> - 2016-08-12 23:50 +0200
Re: [PATCH] asus-laptop: get rid of parse_arg() Giedrius Statkevičius <giedrius.statkevicius@gmail.com> - 2016-08-16 12:00 +0200
Re: [PATCH] asus-laptop: get rid of parse_arg() Darren Hart <dvhart@infradead.org> - 2016-08-17 20:30 +0200
| From | Darren Hart <dvhart@infradead.org> |
|---|---|
| Date | 2016-08-12 23:50 +0200 |
| Subject | Re: [PATCH] asus-laptop: get rid of parse_arg() |
| Message-ID | <s5suZ-7jU-7@gated-at.bofh.it> |
On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > function instead. There is no funcionality change except that in the
> > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > which is not bad because -ERANGE makes more sense here. The check for
> > > !count is already done by the sysfs core so no need to duplicate it
> > > again. Also, add some minor corrections to error handling to accommodate
> > > the change in return values (parse_arg returned count if everything
> > > succeeded whereas kstrtoint returns 0 in the same situation)
> > >
> > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > function old new delta
> > > __UNIQUE_ID_vermagic0 69 70 +1
> > > ls_switch_store 133 117 -16
> > > ledd_store 175 159 -16
> > > display_store 157 141 -16
> > > ls_level_store 193 176 -17
> > > gps_store 200 178 -22
> > > sysfs_acpi_set.isra 148 125 -23
> > > parse_arg.part 39 - -39
> > > Total: Before=19160, After=19012, chg -0.77%
> > >
> > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > ---
> > > drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > 1 file changed, 36 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > index 15f1311..28551f5 100644
> > > --- a/drivers/platform/x86/asus-laptop.c
> > > +++ b/drivers/platform/x86/asus-laptop.c
> > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > }
> > > static DEVICE_ATTR_RO(infos);
> > >
> > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > -{
> > > - if (!count)
> > > - return 0;
> > > - if (count > 31)
> > > - return -EINVAL;
> > > - if (sscanf(buf, "%i", val) != 1)
> > > - return -EINVAL;
> > > - return count;
> > > -}
> > > -
> > > static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > const char *buf, size_t count,
> > > const char *method)
> > > {
> > > int rv, value;
> > >
> > > - rv = parse_arg(buf, count, &value);
> > > - if (rv <= 0)
> > > + rv = kstrtoint(buf, 0, &value);
> > > + if (rv < 0)
> > > return rv;
> > >
> > > if (write_acpi_int(asus->handle, method, value))
> > > return -ENODEV;
> > > - return rv;
> > > + return count;
> >
> > This makes explicit what was hidden before - count is merely a range check, it
> > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > it may succeed and return 12. I suppose this is a failure in the caller, and
> > doesn't impact this function - unless the caller isn't expected to properly
> > terminate the string... but if that were the case, it would have failed
> > previously as we didn't check for that in parse_arg either.... this is fine as
> > is I suppose - can be addressed separately if need be.
> >
OK, good - thanks for the context.
> According to Documentation/filesystems/sysfs.txt:
> "On write(2), ... A terminating null is added after the data on stores. This
> makes functions like sysfs_streq() safe to use."
> So it should be guaranteed that the buffer is a proper C string. Also, we could
> say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> (as it says in the documentation) as it was before this patch (parse_int
> returned count if everything succeeded).
>
> > > }
> > >
> > > /*
> > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > struct asus_laptop *asus = dev_get_drvdata(dev);
> > > int rv, value;
> > >
> > > - rv = parse_arg(buf, count, &value);
> > > - if (rv > 0) {
> > > - if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > - pr_warn("LED display write failed\n");
> > > - return -ENODEV;
> > > - }
> > > - asus->ledd_status = (u32) value;
> > > + rv = kstrtoint(buf, 0, &value);
> > > + if (rv < 0)
> > > + return rv;
> > >
> >
> > This inverts the check to check for failure (this is preferred), but it does
> > change the successful path to include the value of 0, which was skipped over in
> > the original above.
> >
> > > + if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> >
> > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > above. Please consider, and apply decision to all similar instances below.
> >
> Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> 0 as it would be triggered on success. To be honest I didn't get the idea of
> what you wanted to say is wrong with this patch. Could you elaborate more?
>
Your commit message states there is no functional change, but this changes the
behavior of this function (and others) for the 0 edge case.
Previously if rv == 0, it would not call write_acpi_int(), now it will and set
the ledd_status.
--
Darren Hart
Intel Open Source Technology Center
[toc] | [next] | [standalone]
| From | Giedrius Statkevičius <giedrius.statkevicius@gmail.com> |
|---|---|
| Date | 2016-08-16 12:00 +0200 |
| Message-ID | <s6Jk6-1sx-23@gated-at.bofh.it> |
| In reply to | #1461474 |
On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > function instead. There is no funcionality change except that in the
> > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > !count is already done by the sysfs core so no need to duplicate it
> > > > again. Also, add some minor corrections to error handling to accommodate
> > > > the change in return values (parse_arg returned count if everything
> > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > >
> > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > function old new delta
> > > > __UNIQUE_ID_vermagic0 69 70 +1
> > > > ls_switch_store 133 117 -16
> > > > ledd_store 175 159 -16
> > > > display_store 157 141 -16
> > > > ls_level_store 193 176 -17
> > > > gps_store 200 178 -22
> > > > sysfs_acpi_set.isra 148 125 -23
> > > > parse_arg.part 39 - -39
> > > > Total: Before=19160, After=19012, chg -0.77%
> > > >
> > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > ---
> > > > drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > > 1 file changed, 36 insertions(+), 41 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > index 15f1311..28551f5 100644
> > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > > }
> > > > static DEVICE_ATTR_RO(infos);
> > > >
> > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > -{
> > > > - if (!count)
> > > > - return 0;
> > > > - if (count > 31)
> > > > - return -EINVAL;
> > > > - if (sscanf(buf, "%i", val) != 1)
> > > > - return -EINVAL;
> > > > - return count;
> > > > -}
> > > > -
> > > > static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > const char *buf, size_t count,
> > > > const char *method)
> > > > {
> > > > int rv, value;
> > > >
> > > > - rv = parse_arg(buf, count, &value);
> > > > - if (rv <= 0)
> > > > + rv = kstrtoint(buf, 0, &value);
> > > > + if (rv < 0)
> > > > return rv;
> > > >
> > > > if (write_acpi_int(asus->handle, method, value))
> > > > return -ENODEV;
> > > > - return rv;
> > > > + return count;
> > >
> > > This makes explicit what was hidden before - count is merely a range check, it
> > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > doesn't impact this function - unless the caller isn't expected to properly
> > > terminate the string... but if that were the case, it would have failed
> > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > is I suppose - can be addressed separately if need be.
> > >
>
> OK, good - thanks for the context.
>
> > According to Documentation/filesystems/sysfs.txt:
> > "On write(2), ... A terminating null is added after the data on stores. This
> > makes functions like sysfs_streq() safe to use."
> > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > (as it says in the documentation) as it was before this patch (parse_int
> > returned count if everything succeeded).
> >
> > > > }
> > > >
> > > > /*
> > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > int rv, value;
> > > >
> > > > - rv = parse_arg(buf, count, &value);
> > > > - if (rv > 0) {
> > > > - if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > - pr_warn("LED display write failed\n");
> > > > - return -ENODEV;
> > > > - }
> > > > - asus->ledd_status = (u32) value;
> > > > + rv = kstrtoint(buf, 0, &value);
> > > > + if (rv < 0)
> > > > + return rv;
> > > >
> > >
> > > This inverts the check to check for failure (this is preferred), but it does
> > > change the successful path to include the value of 0, which was skipped over in
> > > the original above.
> > >
> > > > + if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > >
> > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > above. Please consider, and apply decision to all similar instances below.
> > >
> > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > what you wanted to say is wrong with this patch. Could you elaborate more?
> >
>
> Your commit message states there is no functional change, but this changes the
> behavior of this function (and others) for the 0 edge case.
>
> Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> the ledd_status.
>
sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
if (!count)
return 0;
--
Giedrius
[toc] | [prev] | [next] | [standalone]
| From | Darren Hart <dvhart@infradead.org> |
|---|---|
| Date | 2016-08-17 20:30 +0200 |
| Message-ID | <s7dLb-4HE-5@gated-at.bofh.it> |
| In reply to | #1463631 |
On Tue, Aug 16, 2016 at 12:49:50PM +0300, Giedrius Statkevičius wrote:
> On Fri, Aug 12, 2016 at 02:40:02PM -0700, Darren Hart wrote:
> > On Sat, Aug 06, 2016 at 08:00:26PM +0300, Giedrius Statkevičius wrote:
> > > On Fri, Aug 05, 2016 at 04:15:07PM -0700, Darren Hart wrote:
> > > > On Fri, Aug 05, 2016 at 11:57:10PM +0300, Giedrius Statkevičius wrote:
> > > > > parse_arg() duplicates the funcionality of kstrtoint() so use the latter
> > > > > function instead. There is no funcionality change except that in the
> > > > > case of input being too big -ERANGE will be returned instead of -EINVAL
> > > > > which is not bad because -ERANGE makes more sense here. The check for
> > > > > !count is already done by the sysfs core so no need to duplicate it
> > > > > again. Also, add some minor corrections to error handling to accommodate
> > > > > the change in return values (parse_arg returned count if everything
> > > > > succeeded whereas kstrtoint returns 0 in the same situation)
> > > > >
> > > > > As a result of this patch asus-laptop.ko size is reduced by almost 1%:
> > > > > add/remove: 0/1 grow/shrink: 1/6 up/down: 1/-149 (-148)
> > > > > function old new delta
> > > > > __UNIQUE_ID_vermagic0 69 70 +1
> > > > > ls_switch_store 133 117 -16
> > > > > ledd_store 175 159 -16
> > > > > display_store 157 141 -16
> > > > > ls_level_store 193 176 -17
> > > > > gps_store 200 178 -22
> > > > > sysfs_acpi_set.isra 148 125 -23
> > > > > parse_arg.part 39 - -39
> > > > > Total: Before=19160, After=19012, chg -0.77%
> > > > >
> > > > > Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@gmail.com>
> > > > > ---
> > > > > drivers/platform/x86/asus-laptop.c | 77 ++++++++++++++++++--------------------
> > > > > 1 file changed, 36 insertions(+), 41 deletions(-)
> > > > >
> > > > > diff --git a/drivers/platform/x86/asus-laptop.c b/drivers/platform/x86/asus-laptop.c
> > > > > index 15f1311..28551f5 100644
> > > > > --- a/drivers/platform/x86/asus-laptop.c
> > > > > +++ b/drivers/platform/x86/asus-laptop.c
> > > > > @@ -932,30 +932,19 @@ static ssize_t infos_show(struct device *dev, struct device_attribute *attr,
> > > > > }
> > > > > static DEVICE_ATTR_RO(infos);
> > > > >
> > > > > -static int parse_arg(const char *buf, unsigned long count, int *val)
> > > > > -{
> > > > > - if (!count)
> > > > > - return 0;
> > > > > - if (count > 31)
> > > > > - return -EINVAL;
> > > > > - if (sscanf(buf, "%i", val) != 1)
> > > > > - return -EINVAL;
> > > > > - return count;
> > > > > -}
> > > > > -
> > > > > static ssize_t sysfs_acpi_set(struct asus_laptop *asus,
> > > > > const char *buf, size_t count,
> > > > > const char *method)
> > > > > {
> > > > > int rv, value;
> > > > >
> > > > > - rv = parse_arg(buf, count, &value);
> > > > > - if (rv <= 0)
> > > > > + rv = kstrtoint(buf, 0, &value);
> > > > > + if (rv < 0)
> > > > > return rv;
> > > > >
> > > > > if (write_acpi_int(asus->handle, method, value))
> > > > > return -ENODEV;
> > > > > - return rv;
> > > > > + return count;
> > > >
> > > > This makes explicit what was hidden before - count is merely a range check, it
> > > > isn't used in parsing the string... I'm not sure if this is a problem, but it
> > > > caught my interest. If count is passed as 12, but buf only contains 3 character,
> > > > it may succeed and return 12. I suppose this is a failure in the caller, and
> > > > doesn't impact this function - unless the caller isn't expected to properly
> > > > terminate the string... but if that were the case, it would have failed
> > > > previously as we didn't check for that in parse_arg either.... this is fine as
> > > > is I suppose - can be addressed separately if need be.
> > > >
> >
> > OK, good - thanks for the context.
> >
> > > According to Documentation/filesystems/sysfs.txt:
> > > "On write(2), ... A terminating null is added after the data on stores. This
> > > makes functions like sysfs_streq() safe to use."
> > > So it should be guaranteed that the buffer is a proper C string. Also, we could
> > > say kstrtoint() or sscanf() uses all of the buffer so it is safe to return count
> > > (as it says in the documentation) as it was before this patch (parse_int
> > > returned count if everything succeeded).
> > >
> > > > > }
> > > > >
> > > > > /*
> > > > > @@ -975,15 +964,17 @@ static ssize_t ledd_store(struct device *dev, struct device_attribute *attr,
> > > > > struct asus_laptop *asus = dev_get_drvdata(dev);
> > > > > int rv, value;
> > > > >
> > > > > - rv = parse_arg(buf, count, &value);
> > > > > - if (rv > 0) {
> > > > > - if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > > > - pr_warn("LED display write failed\n");
> > > > > - return -ENODEV;
> > > > > - }
> > > > > - asus->ledd_status = (u32) value;
> > > > > + rv = kstrtoint(buf, 0, &value);
> > > > > + if (rv < 0)
> > > > > + return rv;
> > > > >
> > > >
> > > > This inverts the check to check for failure (this is preferred), but it does
> > > > change the successful path to include the value of 0, which was skipped over in
> > > > the original above.
> > > >
> > > > > + if (write_acpi_int(asus->handle, METHOD_LEDD, value)) {
> > > >
> > > > What is value if rv is 0? Perhaps safer/more explicit to test for (rv <= 0)
> > > > above. Please consider, and apply decision to all similar instances below.
> > > >
> > > Yes but in this case 0 indicates success so it doesn't make sense to test for <=
> > > 0 as it would be triggered on success. To be honest I didn't get the idea of
> > > what you wanted to say is wrong with this patch. Could you elaborate more?
> > >
> >
> > Your commit message states there is no functional change, but this changes the
> > behavior of this function (and others) for the 0 edge case.
> >
> > Previously if rv == 0, it would not call write_acpi_int(), now it will and set
> > the ledd_status.
> >
>
> sysfs already takes care of this edge case. parse_arg() returned 0 iff count ==
> 0 but in fs/sysfs/file.c sysfs_kf_write() there already is:
>
> if (!count)
> return 0;
From a defensive programming context, we should be explicit in what we pass on
to other functions and not rely on the caller to filter for us, especially when
it's a simple matter of <= versus < in our comparison.
Please update the changed comparison sites in the patch to have the same end
result as before with respect to what it would do if a 0 were received.
--
Darren Hart
Intel Open Source Technology Center
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web