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?
Thanks,
/ magnus
_______________________________________________
fastboot mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/fastboot
