Re: [PATCH] scanlog_init cleanup, minor fixes

[Michael Ellerman]

On Sun, 2008-03-23 at 17:51 -0500, Nathan Lynch wrote:
 scanlog_init() could use some love.

 diff --git a/arch/powerpc/platforms/pseries/scanlog.c 
 b/arch/powerpc/platforms/pseries/scanlog.c
 index 8e1ef16..e5b0ea8 100644
 --- a/arch/powerpc/platforms/pseries/scanlog.c
 +++ b/arch/powerpc/platforms/pseries/scanlog.c
 @@ -195,31 +195,30 @@ const struct file_operations scanlog_fops = {
  static int __init scanlog_init(void)
  {
   struct proc_dir_entry *ent;
 + void *data;
 + int err = -ENOMEM;
  
   ibm_scan_log_dump = rtas_token(ibm,scan-log-dump);
 - if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE) {
 - printk(KERN_ERR scan-log-dump not implemented on this 
 system\n);
 - return -EIO;
 - }
 + if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE)
 + return -ENODEV;
  
 -ent = create_proc_entry(ppc64/rtas/scan-log-dump,  S_IRUSR, NULL);
 - if (ent) {
 - ent-proc_fops = scanlog_fops;
 - /* Ideally we could allocate a buffer  4G */
 - ent-data = kmalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
 - if (!ent-data) {
 - printk(KERN_ERR Failed to allocate a buffer\n);
 - remove_proc_entry(scan-log-dump, ent-parent);
 - return -ENOMEM;
 - }
 - ((unsigned int *)ent-data)[0] = 0;
 - } else {
 - printk(KERN_ERR Failed to create ppc64/scan-log-dump proc 
 entry\n);
 - return -EIO;
 - }
 + /* Ideally we could allocate a buffer  4G */
 + data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
 + if (!data)
 + goto err;

Not your bug, but what happens if data is  4G? Kaboom?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] scanlog_init cleanup, minor fixes

[Michael Ellerman]

On Wed, 2008-03-26 at 18:09 -0500, Nathan Lynch wrote:
 Michael Ellerman wrote:
   + /* Ideally we could allocate a buffer  4G */
   + data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
   + if (!data)
   + goto err;
  
  Not your bug, but what happens if data is  4G? Kaboom?
 
 An old RPA doc (scan-log-dump isn't specified in PAPR) says the buffer
 should be contiguous real storage, so... yeah, probably.  That's why
 I preserved the comment.  Will fix if I get access to a machine to
 test this code more thoroughly (plenty of other issues in this file,
 too).

Cool, no point changing it if we can't test it.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person


signature.asc
Description: This is a digitally signed message part
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

[PATCH] scanlog_init cleanup, minor fixes

[Nathan Lynch]

scanlog_init() could use some love.

* properly return -ENODEV if this system doesn't support scan-log-dump
* don't printk if scan-log-dump not present; only older systems have it
* convert from create_proc_entry() to preferred proc_create()
* allocate zeroed data buffer
* fix potential memory leak of ent-data on failed create_proc_entry()
* simplify control flow

Signed-off-by: Nathan Lynch [EMAIL PROTECTED]
---

(not urgent, please consider for 2.6.26)

 arch/powerpc/platforms/pseries/scanlog.c |   37 ++---
 1 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/scanlog.c 
b/arch/powerpc/platforms/pseries/scanlog.c
index 8e1ef16..e5b0ea8 100644
--- a/arch/powerpc/platforms/pseries/scanlog.c
+++ b/arch/powerpc/platforms/pseries/scanlog.c
@@ -195,31 +195,30 @@ const struct file_operations scanlog_fops = {
 static int __init scanlog_init(void)
 {
struct proc_dir_entry *ent;
+   void *data;
+   int err = -ENOMEM;
 
ibm_scan_log_dump = rtas_token(ibm,scan-log-dump);
-   if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE) {
-   printk(KERN_ERR scan-log-dump not implemented on this 
system\n);
-   return -EIO;
-   }
+   if (ibm_scan_log_dump == RTAS_UNKNOWN_SERVICE)
+   return -ENODEV;
 
-ent = create_proc_entry(ppc64/rtas/scan-log-dump,  S_IRUSR, NULL);
-   if (ent) {
-   ent-proc_fops = scanlog_fops;
-   /* Ideally we could allocate a buffer  4G */
-   ent-data = kmalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
-   if (!ent-data) {
-   printk(KERN_ERR Failed to allocate a buffer\n);
-   remove_proc_entry(scan-log-dump, ent-parent);
-   return -ENOMEM;
-   }
-   ((unsigned int *)ent-data)[0] = 0;
-   } else {
-   printk(KERN_ERR Failed to create ppc64/scan-log-dump proc 
entry\n);
-   return -EIO;
-   }
+   /* Ideally we could allocate a buffer  4G */
+   data = kzalloc(RTAS_DATA_BUF_SIZE, GFP_KERNEL);
+   if (!data)
+   goto err;
+
+   ent = proc_create(ppc64/rtas/scan-log-dump, S_IRUSR, NULL,
+ scanlog_fops);
+   if (!ent)
+   goto err;
+
+   ent-data = data;
proc_ppc64_scan_log_dump = ent;
 
return 0;
+err:
+   kfree(data);
+   return err;
 }
 
 static void __exit scanlog_cleanup(void)
-- 
1.5.4.rc5

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev