Re: [PATCH] Move memory range variables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On 3/15/07, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
> On Thu, Mar 15, 2007 at 06:11:55PM +0900, Magnus Damm wrote:
> > On Thu, 2007-03-15 at 14:30 +0530, Vivek Goyal wrote:
> > > On Thu, Mar 15, 2007 at 05:53:19PM +0900, Magnus Damm wrote:
> > > > Move memory range variables
> > > >
> > > > The common kexec code is currently using two global variables to keep
> > > > track of memory ranges. Other data is kept in a per-instance structure.
> > > > This mix is of per-instance and global variables is confusing and leads
> > > > to messy code in general. So let's not.
> > > >
> > >
> > > These two variable are static and limited to usage by this file only.
> > > Moving to kexec_info makes them visible to everybody and they don't have
> > > to.
> > >
> > > Not sure how does it make code better. I think we don't have to do this.
> >
> > Yes, the variables are limited to that file only. But I do think it is
> > bad coding practice to use a mix of static/global and per-instance
> > variables. And it is being done all over kexec-tools.
> >
> > Such coding practice IMO leads to messy code and people cooking pasta by
> > calling stuff from everywhere. For instance, try to follow the setup and
> > use of the structure memory_range in kexec-ia64.c. It's a mess.
> That's fine but that's the fundamental difference between global variables
> and static global variables. What you seem to be suggesting that lets take
> out all the static global variables and make them pure global by putting
> in a common structure which is passed around. IMHO, does not make much
> sense to me. If some variables are limited to a file and not truly global,
> we don't have to make them visible to everybody. This will also lead to
> unrequired burden on stack as so many functions pass around kexec_info.

I do prefer per-instance data over globals, but that doesn't matter
that much. I think the key issue here is that we should try to stick
to one solution. Doing both is just bad.

Also, the stack thing is a non-issue IMO. This is not kernel code so
we don't have any fixed size stack, and we don't have any issues with
performance so far. And if/when we run into performance issues it is
most likely because of primitive data structures that doesn't scale.

I think we should just optimize for readability.

Maybe it is not just a question of global variables vs per-instance
data. The kexec-tools code looks like functions have been added after
each other in some kind of iterative process - which is all good - but
no one has really taken the time to clean up. And then multiple
architecture support has been added on top of that which unfortunately
just duplicated the buggy code leading to even more bugs.

I'm trying to clean up the code. There are clear bugs all over the
place. Array overruns. One-off memory range bugs. Duplicated code.
Memory leaks. Functions with interesting side effects (global
variables). I wouldn't be surprised if the code relies on the bugs to
work properly as well. So we need to be really careful and make well
tested releases between every batch of fixes.

Thanks for your feedback Vivek!

/ magnus
fastboot mailing list

[Netdev]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Photo]     [Yosemite]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Linux Media]     [Linux Resources]

Powered by Linux