Re: [PATCH 34/40] atm: simplify procfs code
Christoph Hellwigwrites: > On Sat, May 05, 2018 at 07:51:18AM -0500, Eric W. Biederman wrote: >> Christoph Hellwig writes: >> >> > Use remove_proc_subtree to remove the whole subtree on cleanup, and >> > unwind the registration loop into individual calls. Switch to use >> > proc_create_seq where applicable. >> >> Can you please explain why you are removing the error handling when >> you are unwinding the registration loop? > > Because there is no point in handling these errors. The code work > perfectly fine without procfs, or without given proc files and the > removal works just fine if they don't exist either. This is a very > common patter in various parts of the kernel already. > > I'll document it better in the changelog. Thank you. That is the kind of thing that could be a signal of inattentiveness and problems, especially when it is not documented. Eric ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 34/40] atm: simplify procfs code
On Sat, May 05, 2018 at 07:51:18AM -0500, Eric W. Biederman wrote: > Christoph Hellwigwrites: > > > Use remove_proc_subtree to remove the whole subtree on cleanup, and > > unwind the registration loop into individual calls. Switch to use > > proc_create_seq where applicable. > > Can you please explain why you are removing the error handling when > you are unwinding the registration loop? Because there is no point in handling these errors. The code work perfectly fine without procfs, or without given proc files and the removal works just fine if they don't exist either. This is a very common patter in various parts of the kernel already. I'll document it better in the changelog. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 34/40] atm: simplify procfs code
Christoph Hellwigwrites: > Use remove_proc_subtree to remove the whole subtree on cleanup, and > unwind the registration loop into individual calls. Switch to use > proc_create_seq where applicable. Can you please explain why you are removing the error handling when you are unwinding the registration loop? > int __init atm_proc_init(void) > { > - static struct atm_proc_entry *e; > - int ret; > - > atm_proc_root = proc_net_mkdir(_net, "atm", init_net.proc_net); > if (!atm_proc_root) > - goto err_out; > - for (e = atm_proc_ents; e->name; e++) { > - struct proc_dir_entry *dirent; > - > - dirent = proc_create(e->name, 0444, > - atm_proc_root, e->proc_fops); > - if (!dirent) > - goto err_out_remove; > - e->dirent = dirent; > - } > - ret = 0; > -out: > - return ret; > - > -err_out_remove: > - atm_proc_dirs_remove(); > -err_out: > - ret = -ENOMEM; > - goto out; > + return -ENOMEM; > + proc_create_seq("devices", 0444, atm_proc_root, _dev_seq_ops); > + proc_create("pvc", 0444, atm_proc_root, _seq_fops); > + proc_create("svc", 0444, atm_proc_root, _seq_fops); > + proc_create("vc", 0444, atm_proc_root, _seq_fops); > + return 0; > } These proc entries could fail to register with -ENOMEM if for no other reason. Can you justify the removal of the error handling? Can you please at least mention that removal in your change description and explain why it is reasonable. As it sits this is not a semantics preserving transformation, and the difference is not documented. Which raises red flags for me. Eric ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel