Re: [PATCH 2/6] parport: Remove register_sysctl_table from parport_proc_register

[Luis Chamberlain]

On Tue, May 16, 2023 at 06:12:21PM +0200, Joel Granados wrote:
> On Tue, May 16, 2023 at 04:31:16PM +0200, Joel Granados wrote:
> > On Mon, May 15, 2023 at 09:17:39PM -0700, Luis Chamberlain wrote:
> > > Awesome!
> > > 
> > > On Mon, May 15, 2023 at 09:14:42AM +0200, Joel Granados wrote:
> > > > +
> > > > +	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 = kmalloc(tmp_path_len, GFP_KERNEL);
> > > 
> > > Any reason why not kzalloc()?
> > nope. Will zero it out.
> > 
> > > 
> > > > +	if (!tmp_dir_path) {
> > > > +		err = -ENOMEM;
> > > > +		goto exit_free_t;
> > > > +	}
> > > >  
> > > > -	t->port_dir[0].procname = port->name;
> > > > +	if (tmp_path_len
> > > > +	    <= snprintf(tmp_dir_path, tmp_path_len, "dev/parport/%s/devices", port->name)) {
> > > 
> > > Since we are clearing up obfuscation code, it would be nicer to
> > > make this easier to read and split the snprintf() into one line, capture
> > > the error there. And then in a new line do the check. Even if we have to
> > > add a new int value here.
> > np. Will do something like this:
> > 
> > num_chars_sprinted = snprintf(....
> > if(tmp_path_len <= num_chars_sprinted) {
> >   err = -ENOENT;
> >   ...
> > }
> > 
> > > 
> > > Other than this I'd just ask to extend the commit log to use
> > > the before and after of vmlinux (when this module is compiled in with all
> > > the bells and whistles) with ./scripts/bloat-o-meter.
> > > 
> > > Ie build before the patch and cp vmlinux to vmlinux.old and then compare
> > > with:
> > > 
> > > ./scripts/bloat-o-meter vmlinux.old vmlinux
> > > 
> > > Can you also describe any testing if any.
> > Sure thing. Will add the bloat-o-meter output on the last patch so as to
> > gather the results for all the patches.
> > 
> > I'll write some testing info on the patches.
> > 
> > 
> > > 
> > > With the above changes, feel free to add to all these patches:
> > > 
> > > Reviewed-by: Luis Chamberlain
> > Ack
> > 
> > > 
> > > > +	if (register_sysctl(tmp_dir_path, t->device_dir) == NULL)
> > > 
> > > BTW, we should be able to remove now replace register_sysctl_base() with a simple
> > > register_sysctl("kernel", foo), and then one for "fs", and one of "vm"
> > > on kernel/sysctl.c and just remove:
> > > 
> > >   * DECLARE_SYSCTL_BASE() & register_sysctl_base() & __register_sysctl_base()
> > >   * and then after all this register_sysctl_table() completely
> > > 
> > > Let me know if you'd like a stab at it, or if you prefer me to do that.
> > I think I can give it a go. Should I just add that to these set of
> > patches? or should we create a new patch set?
> I'll send the V2 of this patch set without this. Will add it to the
> patch set when I finish if it makes sense. Else I'll just create a new
> series.

New series is good.

  Luis