Hi Eric,
Thanks for you comment.
On 11/23/06, Eric W. Biederman <[EMAIL PROTECTED]> wrote:
> "Magnus Damm" <[EMAIL PROTECTED]> writes:
>
> > On 11/22/06, Vivek Goyal <[EMAIL PROTECTED]> wrote:
> >> On Tue, Nov 21, 2006 at 03:25:39PM +0900, Magnus Damm wrote:
> >> [..]
> >> > >> +int kexec_iomem_single(char *str, uint64_t *start, uint64_t *end)
> >> > >> +{
> >> > >
> >> > >Would "parse_iomem_single" be a better name for the function?
> >> >
> >> > I usually name my functions with the filename as prefix - hence the
> >> > "kexec_iomem". But I don't mind changing the name - my main reason
> >> > behind this is to reduce code redundancy so I'm willing to give in on
> >> > almost everything except on code removal. =)
> >> >
> >> > >> + struct memory_range range;
> >> > >> + int ret;
> >> > >> +
> >> > >> + memset(&range, 0, sizeof(range));
> >> > >> +
> >> > >> + ret = kexec_iomem_for_each_line(str,
> >> > >> kexec_iomem_single_callback,
> >> > >> + &range);
> >> > >> +
> >> > >> + if (ret == 1) {
> >> > >
> >> > >I think everywhere now we are using the convention that return code 0 is
> >> > >success otherwise its failure. Can we stick to that convention.
> >> >
> >> > Well, all functions except kexec_iomem_for_each_line() use that
> >> > convention. The kexec_iomem_for_each_line() code returns the number of
> >> > matched lines, and the Xen code does rely on that functionality today.
> >> >
> >> > I can of course change the code but that will result in some code
> >> > duplication. =) Maybe it is ok as is today now when you know the
> >> > reason. Is adding a comment to describe the function and return value
> >> > good enough?
> >>
> >> If it is not too much of code, can't we return the number of lines matched
> >> in a parameter and keep the return value to continue to function as error
> >> code. IMHO, it is good to be consistent with rest of the functions.
> >
> > Sure, either way is ok with me, it does not matter. But what does
> > matter to me is to get the code merged, so maybe we can add this as a
> > separate fix on top of my code?
>
>
> If you start playing fancy games with /proc/iomem parsing I suggest
> some one write a simple general parser that stores the parsed file
> in an array. With elements something like:
>
> unsigned long long start;
> unsigned long long end;
> char *type;
>
> Then just have all of the people who need to get creative walk the
> stupid array.
>
> That doesn't need callback or any other complications. Just simple
> code doing simple things.
My xen code does in the case of the crash notes, but using the callback:
http://lists.osdl.org/pipermail/fastboot/2006-November/005138.html
The reason why I prefer a callback is that I feel that exporting an
array just encourages sloppy programming which leads to messy code. A
good example of such code IMO is the code that deals with the global
variable crash_memory_range[]. And that code happens to do exactly
what you suggest.
I think using a callback leads to cleaner code over time, but callback
or array is not the big issue here. My main concern is the amount of
duplicated code. Having a either a single callback implementation or a
single array implementation is much better than duplicated code...
Thanks,
/ magnus
_______________________________________________
fastboot mailing list
fastboot@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/fastboot
