| Message ID | 1377114413-8521-2-git-send-email-sebastian.capella@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi! > Use the name_to_dev_t call to parse the device name echo'd to > to /sys/power/resume. This imitates the method used in hibernate.c > in software_resume, and allows the resume partition to be specified > using other equivalent device formats as well. By allowing > /sys/debug/resume to accept the same syntax as the resume=device > parameter, we can parse the resume=device in the init script and > use the resume device directly from the kernel command line. > > Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> > --- > kernel/power/hibernate.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c > index b26f5f1..51d4c29 100644 > --- a/kernel/power/hibernate.c > +++ b/kernel/power/hibernate.c > @@ -971,15 +971,19 @@ static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, > static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, > const char *buf, size_t n) > { > - unsigned int maj, min; > dev_t res; > int ret = -EINVAL; > + int len = n; > + char *devcpy; > > - if (sscanf(buf, "%u:%u", &maj, &min) != 2) > - goto out; > + if (buf[len-1] == '\n') > + len--; > + > + devcpy = kstrndup(buf, len, GFP_KERNEL); > + res = name_to_dev_t(devcpy); > + kfree(devcpy); Is the allocation actually neccessary? At the very least this should test for NULL... Pavel
On Mon 2013-08-26 10:40:50, Sebastian Capella wrote: > Apologies for my previous top post reply... > > Quoting Pavel Machek (2013-08-25 08:38:11) > > Is the allocation actually neccessary? At the very least this should > > test for NULL... > > > Thanks Pavel! I'll add the check for NULL. > > name_to_dev_t expects a non-const name, but the buffer passed in > is const. I also am removing the '\n' if found at the end of the > string which would violate the const. Fix name_to_dev_t, then. No need to do memory allocation just to work around const. [You can also take a look why it is const in the first place. I don't think it needs to be.] Pavel
On Tue 2013-09-17 13:50:21, Sebastian Capella wrote: > Quoting Sebastian Capella (2013-08-30 11:42:30) > > Quoting Pavel Machek (2013-08-30 04:35:33) > > > On Mon 2013-08-26 10:40:50, Sebastian Capella wrote: > > > > Quoting Pavel Machek (2013-08-25 08:38:11) > > > > > Is the allocation actually neccessary? At the very least this should > > > > > test for NULL... > > > > > > > > name_to_dev_t expects a non-const name, but the buffer passed in > > > > is const. I also am removing the '\n' if found at the end of the > > > > string which would violate the const. > > > > > > Fix name_to_dev_t, then. No need to do memory allocation just to work > > > around const. > > > > > Hi Pavel, > > > > The issue is really Removing the \n from the user space input. The > > flow is: > > const input buf -> copy to work buffer, remove newline -> name_to_dev_t > > > > ssize_t resume_store(..., const char *buf, size_t n) > > // copy buf, strip off trailing newline, pass to name_to_dev_t > > dev_t name_to_dev_t(char *name) > > > > The const in the restore_store buffer comes from the function type of the > > store member of the kobj_attribute. I don't believe this should be changed. > > > > Currently, name_to_dev_t will fail in some cases if a trailing \n is present. > > Is it more appropriate to handle stripping the newline in the store > > function rather than modifying name_to_dev_t to clean it up? > > > > It seems logical for name_to_dev_t to take a const name parameter as > > there should be no reason to modify the name buffer passed to it. > > I'll be happy to make a patch to do this, but without hardening > > name_to_dev_t against trailing newlines, it would not be neccesary for > > this problem. > > > > Thanks for your time and comments! > > > > Hi Pavel, > > Do you have any more feedback regarding leaving the strndup? I think you should modify name_to_dev_t, then. Doing memory allocation just to work around \n limitation is ugly. Pavel
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c index b26f5f1..51d4c29 100644 --- a/kernel/power/hibernate.c +++ b/kernel/power/hibernate.c @@ -971,15 +971,19 @@ static ssize_t resume_show(struct kobject *kobj, struct kobj_attribute *attr, static ssize_t resume_store(struct kobject *kobj, struct kobj_attribute *attr, const char *buf, size_t n) { - unsigned int maj, min; dev_t res; int ret = -EINVAL; + int len = n; + char *devcpy; - if (sscanf(buf, "%u:%u", &maj, &min) != 2) - goto out; + if (buf[len-1] == '\n') + len--; + + devcpy = kstrndup(buf, len, GFP_KERNEL); + res = name_to_dev_t(devcpy); + kfree(devcpy); - res = MKDEV(maj,min); - if (maj != MAJOR(res) || min != MINOR(res)) + if (res == 0) goto out; lock_system_sleep();
Use the name_to_dev_t call to parse the device name echo'd to to /sys/power/resume. This imitates the method used in hibernate.c in software_resume, and allows the resume partition to be specified using other equivalent device formats as well. By allowing /sys/debug/resume to accept the same syntax as the resume=device parameter, we can parse the resume=device in the init script and use the resume device directly from the kernel command line. Signed-off-by: Sebastian Capella <sebastian.capella@linaro.org> --- kernel/power/hibernate.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)