* [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting @ 2023-09-01 13:42 ` Andy Shevchenko 2023-09-01 13:42 ` [PATCH v1 2/3] parport: Use list_for_each() helper Andy Shevchenko ` (3 more replies) 0 siblings, 4 replies; 6+ messages in thread From: Andy Shevchenko @ 2023-09-01 13:42 UTC (permalink / raw) To: Luis Chamberlain, Joel Granados, Andy Shevchenko, linux-kernel Cc: Sudip Mukherjee Improve readability and maintainability by replacing a hardcoded string allocation and formatting by the use of the kasprintf() helper. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/parport/procfs.c | 53 +++++++--------------------------------- drivers/parport/share.c | 15 +++++------- include/linux/parport.h | 2 -- 3 files changed, 15 insertions(+), 55 deletions(-) diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c index 4e5b972c3e26..7aa99c65b934 100644 --- a/drivers/parport/procfs.c +++ b/drivers/parport/procfs.c @@ -32,13 +32,6 @@ #define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ) #define PARPORT_MIN_SPINTIME_VALUE 1 #define PARPORT_MAX_SPINTIME_VALUE 1000 -/* - * PARPORT_BASE_* is the size of the known parts of the sysctl path - * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9 - * and null char(1). - */ -#define PARPORT_BASE_PATH_SIZE 13 -#define PARPORT_BASE_DEVICES_PATH_SIZE 22 static int do_active_device(struct ctl_table *table, int write, void *result, size_t *lenp, loff_t *ppos) @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port) { struct parport_sysctl_table *t; char *tmp_dir_path; - size_t tmp_path_len, port_name_len; - int bytes_written, i, err = 0; + int i, err = 0; t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL); if (t == NULL) @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port) t->vars[5 + i].extra2 = &port->probe_info[i]; } - port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN); - /* - * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices. - * We calculate for the second as that will give us enough for the first. - */ - tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len; - tmp_dir_path = kzalloc(tmp_path_len, GFP_KERNEL); + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name); if (!tmp_dir_path) { err = -ENOMEM; goto exit_free_t; } - bytes_written = snprintf(tmp_dir_path, tmp_path_len, - "dev/parport/%s/devices", port->name); - if (tmp_path_len <= bytes_written) { - err = -ENOENT; - goto exit_free_tmp_dir_path; - } t->devices_header = register_sysctl(tmp_dir_path, t->device_dir); if (t->devices_header == NULL) { err = -ENOENT; goto exit_free_tmp_dir_path; } - tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len; - bytes_written = snprintf(tmp_dir_path, tmp_path_len, - "dev/parport/%s", port->name); - if (tmp_path_len <= bytes_written) { - err = -ENOENT; + kfree(tmp_dir_path); + + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name); + if (!tmp_dir_path) { + err = -ENOMEM; goto unregister_devices_h; } @@ -514,34 +494,22 @@ int parport_proc_unregister(struct parport *port) int parport_device_proc_register(struct pardevice *device) { - int bytes_written, err = 0; struct parport_device_sysctl_table *t; struct parport * port = device->port; - size_t port_name_len, device_name_len, tmp_dir_path_len; char *tmp_dir_path; + int err = 0; t = kmemdup(&parport_device_sysctl_template, sizeof(*t), GFP_KERNEL); if (t == NULL) return -ENOMEM; - port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN); - device_name_len = strnlen(device->name, PATH_MAX); - /* Allocate a buffer for two paths: dev/parport/PORT/devices/DEVICE. */ - tmp_dir_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len + device_name_len; - tmp_dir_path = kzalloc(tmp_dir_path_len, GFP_KERNEL); + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices/%s", port->name, device->name); if (!tmp_dir_path) { err = -ENOMEM; goto exit_free_t; } - bytes_written = snprintf(tmp_dir_path, tmp_dir_path_len, "dev/parport/%s/devices/%s", - port->name, device->name); - if (tmp_dir_path_len <= bytes_written) { - err = -ENOENT; - goto exit_free_path; - } - t->vars[0].data = &device->timeslice; t->sysctl_header = register_sysctl(tmp_dir_path, t->vars); @@ -554,9 +522,6 @@ int parport_device_proc_register(struct pardevice *device) kfree(tmp_dir_path); return 0; -exit_free_path: - kfree(tmp_dir_path); - exit_free_t: kfree(t); diff --git a/drivers/parport/share.c b/drivers/parport/share.c index 2d46b1d4fd69..8037bcd07bcf 100644 --- a/drivers/parport/share.c +++ b/drivers/parport/share.c @@ -438,7 +438,6 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, struct parport *tmp; int num; int device; - char *name; int ret; tmp = kzalloc(sizeof(struct parport), GFP_KERNEL); @@ -467,11 +466,6 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, atomic_set(&tmp->ref_count, 1); INIT_LIST_HEAD(&tmp->full_list); - name = kmalloc(PARPORT_NAME_MAX_LEN, GFP_KERNEL); - if (!name) { - kfree(tmp); - return NULL; - } /* Search for the lowest free parport number. */ spin_lock(&full_list_lock); @@ -487,11 +481,14 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, /* * Now that the portnum is known finish doing the Init. */ - sprintf(name, "parport%d", tmp->portnum = tmp->number); - tmp->name = name; + tmp->name = kasprintf(GFP_KERNEL, "parport%d", tmp->portnum); + if (!tmp->name) { + kfree(tmp); + return NULL; + } + dev_set_name(&tmp->bus_dev, tmp->name); tmp->bus_dev.bus = &parport_bus_type; tmp->bus_dev.release = free_port; - dev_set_name(&tmp->bus_dev, name); tmp->bus_dev.type = &parport_device_type; for (device = 0; device < 5; device++) diff --git a/include/linux/parport.h b/include/linux/parport.h index 999eddd619b7..fff39bc30629 100644 --- a/include/linux/parport.h +++ b/include/linux/parport.h @@ -180,8 +180,6 @@ struct ieee1284_info { struct semaphore irq; }; -#define PARPORT_NAME_MAX_LEN 15 - /* A parallel port */ struct parport { unsigned long base; /* base address */ -- 2.40.0.1.gaa8946217a0b ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v1 2/3] parport: Use list_for_each() helper 2023-09-01 13:42 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Andy Shevchenko @ 2023-09-01 13:42 ` Andy Shevchenko 2023-09-01 13:42 ` [PATCH v1 3/3] parport: Drop unneeded NULL or 0 assignments Andy Shevchenko ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2023-09-01 13:42 UTC (permalink / raw) To: Luis Chamberlain, Joel Granados, Andy Shevchenko, linux-kernel Cc: Sudip Mukherjee Convert hard to read custom code to list_for_each(). No functional changes intended. Note, we may not use list_for_each_entry() as at the end of the list the iterator will point to an invalid entry and may not be dereferenced. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/parport/share.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/parport/share.c b/drivers/parport/share.c index 8037bcd07bcf..38780f6a9119 100644 --- a/drivers/parport/share.c +++ b/drivers/parport/share.c @@ -469,9 +469,11 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, /* Search for the lowest free parport number. */ spin_lock(&full_list_lock); - for (l = all_ports.next, num = 0; l != &all_ports; l = l->next, num++) { + num = 0; + list_for_each(l, &all_ports) { struct parport *p = list_entry(l, struct parport, full_list); - if (p->number != num) + + if (p->number != num++) break; } tmp->portnum = tmp->number = num; -- 2.40.0.1.gaa8946217a0b ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v1 3/3] parport: Drop unneeded NULL or 0 assignments 2023-09-01 13:42 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Andy Shevchenko 2023-09-01 13:42 ` [PATCH v1 2/3] parport: Use list_for_each() helper Andy Shevchenko @ 2023-09-01 13:42 ` Andy Shevchenko 2023-09-04 13:11 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Joel Granados 2023-10-02 13:06 ` Andy Shevchenko 3 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2023-09-01 13:42 UTC (permalink / raw) To: Luis Chamberlain, Joel Granados, Andy Shevchenko, linux-kernel Cc: Sudip Mukherjee kzalloc() gives us a zeroed memory, no need to explicitly assing 0 or NULL or similar to the members of the data structure that has been allocated with the above mentioned API. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/parport/share.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/drivers/parport/share.c b/drivers/parport/share.c index 38780f6a9119..a9a9cb0477ea 100644 --- a/drivers/parport/share.c +++ b/drivers/parport/share.c @@ -449,13 +449,9 @@ struct parport *parport_register_port(unsigned long base, int irq, int dma, tmp->irq = irq; tmp->dma = dma; tmp->muxport = tmp->daisy = tmp->muxsel = -1; - tmp->modes = 0; INIT_LIST_HEAD(&tmp->list); - tmp->devices = tmp->cad = NULL; - tmp->flags = 0; tmp->ops = ops; tmp->physport = tmp; - memset(tmp->probe_info, 0, 5 * sizeof(struct parport_device_info)); rwlock_init(&tmp->cad_lock); spin_lock_init(&tmp->waitlist_lock); spin_lock_init(&tmp->pardevice_lock); -- 2.40.0.1.gaa8946217a0b ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting 2023-09-01 13:42 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Andy Shevchenko 2023-09-01 13:42 ` [PATCH v1 2/3] parport: Use list_for_each() helper Andy Shevchenko 2023-09-01 13:42 ` [PATCH v1 3/3] parport: Drop unneeded NULL or 0 assignments Andy Shevchenko @ 2023-09-04 13:11 ` Joel Granados 2023-09-04 13:44 ` Andy Shevchenko 2023-10-02 13:06 ` Andy Shevchenko 3 siblings, 1 reply; 6+ messages in thread From: Joel Granados @ 2023-09-04 13:11 UTC (permalink / raw) To: Andy Shevchenko; +Cc: Luis Chamberlain, linux-kernel, Sudip Mukherjee [-- Attachment #1.1: Type: text/plain, Size: 4256 bytes --] On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote: > Improve readability and maintainability by replacing a hardcoded string > allocation and formatting by the use of the kasprintf() helper. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/parport/procfs.c | 53 +++++++--------------------------------- > drivers/parport/share.c | 15 +++++------- > include/linux/parport.h | 2 -- > 3 files changed, 15 insertions(+), 55 deletions(-) > > diff --git a/drivers/parport/procfs.c b/drivers/parport/procfs.c > index 4e5b972c3e26..7aa99c65b934 100644 > --- a/drivers/parport/procfs.c > +++ b/drivers/parport/procfs.c > @@ -32,13 +32,6 @@ > #define PARPORT_MAX_TIMESLICE_VALUE ((unsigned long) HZ) > #define PARPORT_MIN_SPINTIME_VALUE 1 > #define PARPORT_MAX_SPINTIME_VALUE 1000 > -/* > - * PARPORT_BASE_* is the size of the known parts of the sysctl path > - * in dev/partport/%s/devices/%s. "dev/parport/"(12), "/devices/"(9 > - * and null char(1). > - */ > -#define PARPORT_BASE_PATH_SIZE 13 > -#define PARPORT_BASE_DEVICES_PATH_SIZE 22 > > static int do_active_device(struct ctl_table *table, int write, > void *result, size_t *lenp, loff_t *ppos) > @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port) > { > struct parport_sysctl_table *t; > char *tmp_dir_path; > - size_t tmp_path_len, port_name_len; > - int bytes_written, i, err = 0; > + int i, err = 0; > > t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL); > if (t == NULL) > @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port) For this function I would even go a step further and start with the two kasprintf calls so we can then free them in the reverse order. And then leave the rest as it is. I attached an untested diff that applies on top of your changes to show you what I mean. > t->vars[5 + i].extra2 = &port->probe_info[i]; > } > > - port_name_len = strnlen(port->name, PARPORT_NAME_MAX_LEN); > - /* > - * Allocate a buffer for two paths: dev/parport/PORT and dev/parport/PORT/devices. > - * We calculate for the second as that will give us enough for the first. > - */ > - tmp_path_len = PARPORT_BASE_DEVICES_PATH_SIZE + port_name_len; > - tmp_dir_path = kzalloc(tmp_path_len, GFP_KERNEL); > + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name); > if (!tmp_dir_path) { > err = -ENOMEM; > goto exit_free_t; > } > > - bytes_written = snprintf(tmp_dir_path, tmp_path_len, > - "dev/parport/%s/devices", port->name); > - if (tmp_path_len <= bytes_written) { > - err = -ENOENT; > - goto exit_free_tmp_dir_path; > - } > t->devices_header = register_sysctl(tmp_dir_path, t->device_dir); > if (t->devices_header == NULL) { > err = -ENOENT; > goto exit_free_tmp_dir_path; > } > > - tmp_path_len = PARPORT_BASE_PATH_SIZE + port_name_len; > - bytes_written = snprintf(tmp_dir_path, tmp_path_len, > - "dev/parport/%s", port->name); > - if (tmp_path_len <= bytes_written) { > - err = -ENOENT; > + kfree(tmp_dir_path); > + > + tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name); > + if (!tmp_dir_path) { > + err = -ENOMEM; > goto unregister_devices_h; > } > > @@ -514,34 +494,22 @@ int parport_proc_unregister(struct parport *port) > > int parport_device_proc_register(struct pardevice *device) > { > - int bytes_written, err = 0; > struct parport_device_sysctl_table *t; > struct parport * port = device->port; > - size_t port_name_len, device_name_len, tmp_dir_path_len; > char *tmp_dir_path; ... > diff --git a/include/linux/parport.h b/include/linux/parport.h > index 999eddd619b7..fff39bc30629 100644 > --- a/include/linux/parport.h > +++ b/include/linux/parport.h > @@ -180,8 +180,6 @@ struct ieee1284_info { > struct semaphore irq; > }; > > -#define PARPORT_NAME_MAX_LEN 15 This variable protected against port->name not ending in '\0'. Anyone worried that kasprintf could be unbounded? > - > /* A parallel port */ > struct parport { > unsigned long base; /* base address */ > -- > 2.40.0.1.gaa8946217a0b > -- Joel Granados [-- Attachment #1.2: parport.patch --] [-- Type: text/x-diff, Size: 2313 bytes --] diff --git i/drivers/parport/procfs.c w/drivers/parport/procfs.c index 7aa99c65b934..0e3b01368aec 100644 --- i/drivers/parport/procfs.c +++ w/drivers/parport/procfs.c @@ -423,12 +423,24 @@ parport_default_sysctl_table = { int parport_proc_register(struct parport *port) { struct parport_sysctl_table *t; - char *tmp_dir_path; + char *parport_name_devices, *parport_name; int i, err = 0; + parport_name_devices = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name); + if (!parport_name_devices) + return -ENOMEM; + + parport_name = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name); + if (!parport_name) { + err = -ENOMEM; + goto exit_partport_name_devices; + } + t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL); - if (t == NULL) - return -ENOMEM; + if (t == NULL) { + err = -ENOMEM; + goto exit_parport_name; + } t->device_dir[0].extra1 = port; @@ -438,27 +450,13 @@ int parport_proc_register(struct parport *port) t->vars[5 + i].extra2 = &port->probe_info[i]; } - tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s/devices", port->name); - if (!tmp_dir_path) { - err = -ENOMEM; - goto exit_free_t; - } - - t->devices_header = register_sysctl(tmp_dir_path, t->device_dir); + t->devices_header = register_sysctl(parport_name_devices, t->device_dir); if (t->devices_header == NULL) { err = -ENOENT; - goto exit_free_tmp_dir_path; + goto exit_free_t; } - kfree(tmp_dir_path); - - tmp_dir_path = kasprintf(GFP_KERNEL, "dev/parport/%s", port->name); - if (!tmp_dir_path) { - err = -ENOMEM; - goto unregister_devices_h; - } - - t->port_header = register_sysctl(tmp_dir_path, t->vars); + t->port_header = register_sysctl(parport_name, t->vars); if (t->port_header == NULL) { err = -ENOENT; goto unregister_devices_h; @@ -466,17 +464,19 @@ int parport_proc_register(struct parport *port) port->sysctl_table = t; - kfree(tmp_dir_path); - return 0; + goto exit_parport_name; unregister_devices_h: unregister_sysctl_table(t->devices_header); -exit_free_tmp_dir_path: - kfree(tmp_dir_path); - exit_free_t: kfree(t); + +exit_parport_name: + kfree(parport_name); + +exit_partport_name_devices: + kfree(parport_name_devices); return err; } [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 659 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting 2023-09-04 13:11 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Joel Granados @ 2023-09-04 13:44 ` Andy Shevchenko 0 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2023-09-04 13:44 UTC (permalink / raw) To: Joel Granados; +Cc: Luis Chamberlain, linux-kernel, Sudip Mukherjee On Mon, Sep 04, 2023 at 03:11:45PM +0200, Joel Granados wrote: > On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote: Thank you for the review, my answers below. ... > > @@ -431,8 +424,7 @@ int parport_proc_register(struct parport *port) > > { > > struct parport_sysctl_table *t; > > char *tmp_dir_path; > > - size_t tmp_path_len, port_name_len; > > - int bytes_written, i, err = 0; > > + int i, err = 0; > > > > t = kmemdup(&parport_sysctl_template, sizeof(*t), GFP_KERNEL); > > if (t == NULL) > > @@ -446,35 +438,23 @@ int parport_proc_register(struct parport *port) > For this function I would even go a step further and start with the two > kasprintf calls so we can then free them in the reverse order. And then > leave the rest as it is. I'm not sure I see the big picture here. Can you draft what you are proposing (showing only the lines that are important)? > I attached an untested diff that applies on > top of your changes to show you what I mean. Ah, I see now, it's below. And how is it better? (LoCs statistics seems to be the same, so...) What is a downside in my opinion with your code is this line return 0; --> goto blablabla. this makes code less maintainable. OTOH we may do what you want, but it will take a bit more LoCs and honestly I don't see the benefit of doing that as in both cases the variable used is temporary. What may be the good solution here is to split the repetitive code excerpt to the parameterized helper function. But this will be another patch which you can build on top of this series, right? ... > > diff --git a/include/linux/parport.h b/include/linux/parport.h > > index 999eddd619b7..fff39bc30629 100644 > > --- a/include/linux/parport.h > > +++ b/include/linux/parport.h > > @@ -180,8 +180,6 @@ struct ieee1284_info { > > struct semaphore irq; > > }; > > > > -#define PARPORT_NAME_MAX_LEN 15 > This variable protected against port->name not ending in '\0'. Anyone > worried that kasprintf could be unbounded? I'm lost here. kasprintf() guarantees the NUL-termination. Any other concerns? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting 2023-09-01 13:42 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Andy Shevchenko ` (2 preceding siblings ...) 2023-09-04 13:11 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Joel Granados @ 2023-10-02 13:06 ` Andy Shevchenko 3 siblings, 0 replies; 6+ messages in thread From: Andy Shevchenko @ 2023-10-02 13:06 UTC (permalink / raw) To: Luis Chamberlain, Joel Granados, linux-kernel; +Cc: Sudip Mukherjee On Fri, Sep 01, 2023 at 04:42:48PM +0300, Andy Shevchenko wrote: > Improve readability and maintainability by replacing a hardcoded string > allocation and formatting by the use of the kasprintf() helper. Sudip, any comments on this series? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-10-02 13:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230901134310eucas1p1d9be610c894d46f19bb6c12576aef94b@eucas1p1.samsung.com>
2023-09-01 13:42 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Andy Shevchenko
2023-09-01 13:42 ` [PATCH v1 2/3] parport: Use list_for_each() helper Andy Shevchenko
2023-09-01 13:42 ` [PATCH v1 3/3] parport: Drop unneeded NULL or 0 assignments Andy Shevchenko
2023-09-04 13:11 ` [PATCH v1 1/3] parport: Use kasprintf() instead of fixed buffer formatting Joel Granados
2023-09-04 13:44 ` Andy Shevchenko
2023-10-02 13:06 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox