Hi Alain,
I finally got the tree with your latest bzip2/LZMA code unburied. My
apologies for having sat on it for so long (you know why.)
Anyway, I did run into a few things that I'd really like to get fixed.
First of all, there are more than just the x86 and ARM tree which is
using the old API, I count at least four more architectures. I can't
apply the ARM tree anyway, and we clearly need a saner migration way.
Second, the ugly if...goto hacks in init/initramfs.c could be much
cleaner handled by iterating down a list of function pointers -- except
that gunzip() seems to have a different return value (in init/than the
others?!
Third, you're using the construct if (!foo() < 0 && message == NULL)
... which is wrong on three(!) accounts, one technical (! has higher
precedence than <) and two stylistic (write >= instead of !(.. < ..) and
write !message instead of message == NULL).
I think the solution to all of this is to introduce a new function
instead of gunzip(), and have the old gunzip() be a wrapper using the
old ABI. That is much cleaner than relying on #ifdef NEW_CODE.
Finally, I know I suggested it, but I don't think the
<linux/decompress/*.h> header file are justified given that they only
contain a single function header. Might as well put them all in a
single <linux/decompress.h> header.
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
H. Peter Anvin wrote:
> Hi Alain,
>
> I finally got the tree with your latest bzip2/LZMA code unburied.
Thanks for finally getting back to it.
> My
> apologies for having sat on it for so long (you know why.)
I think the real problem here really is that there is nobody else who
seems to be able to jump in for you if you're unavailable for a longer
period for whatever reason.
> Anyway, I did run into a few things that I'd really like to get fixed.
>
> First of all, there are more than just the x86 and ARM tree which is
> using the old API, I count at least four more architectures. I can't
> apply the ARM tree anyway, and we clearly need a saner migration way.
Yes, that's why I introduced the NEW_CODE define. Especially since I've
got no way to test for these 4 architectures. Is that #ifdef really that
big of a catastrophe if it actually solves the problem? Especially since
it's only going to be around for a while, and then go away, once those 4
architectures are ported too.
> Second, the ugly if...goto hacks in init/initramfs.c could be much
> cleaner handled by iterating down a list of function pointers -- except
> that gunzip() seems to have a different return value (in init/than the
> others?!
>
> Third, you're using the construct if (!foo() < 0 && message == NULL)
>
> ... which is wrong on three(!) accounts, one technical (! has higher
> precedence than <) and two stylistic (write >= instead of !(.. < ..) and
> write !message instead of message == NULL).
>
> I think the solution to all of this is to introduce a new function
> instead of gunzip(), and have the old gunzip() be a wrapper using the
> old ABI.
The purpose here was to have a change footprint as small as possible, in
order to avoid breaking ugly (but working) code.
The thing is, if you discourage having ugly "old" code in patches, ugly
leftovers from the early days will never be fixed because:
1. Piecemeal approach is prohibited (as that would leave ugly code in patch)
2. "Ugly" code by definition is hard to understand. If you can only get
it fixed by removing it entirely in one go, nobody might dare to change
it, for fear of breaking stuff.
> That is much cleaner than relying on #ifdef NEW_CODE.
Having a wrapper may not be feasible in the pre-boot environment,
because there the code is #included, not linked. You'd have lots of
symbol clashes between the real gunzip's internal variables, and the
ones that are used to pass parameters to the wrapper.
It's feasible, but non-trivial legwork. After all what happened, I'm
really hesitant to spend nights on work that will just sit in a corner
ignored for months, especially if this is by definition for temporary
stuff (in order to cover the period between the time when the patch is
applied for ARM and Intel, and the time when it will be applied to all
architectures). The #ifdef NEW_CODE solves the problem in a clean way
for the purpose. Clean does not necessarily mean "looks nice in an
editor" or "complies to arbitrary 'thouh shalt not #ifdef' rules", but
can also mean "efficiently addresses the problem". Please read up on
agile methodologies.
> Finally, I know I suggested it, but I don't think the
You see, that's exactly why by now I've become increasingly hesitant to
act on your suggestions, especially when they involve non-trivial
amounts of work. I'm getting the feeling that all this is just a charade
to never get the patch accepted.
> <linux/decompress/*.h> header file are justified given that they only
> contain a single function header. Might as well put them all in a
> single <linux/decompress.h> header.
>
> -hpa
>
If I may suggest something: what if *YOU* provided a patch (or series of
patches, or whatever you like) complying to your high stylistic
requirements, and I check it for functionality (whether it is able to
decompress kernels compressed by the 3 methods on the 2 architectures
that I have access to). Maybe that way we can get this moving along faster?
Thanks & Best Wishes for 2009 for Everybody!
Alain
Hi Alain.
>
> If I may suggest something: what if *YOU* provided a patch (or series of
> patches, or whatever you like) complying to your high stylistic
> requirements
hpa does not have any high stylistic requirments he just have good taste.
He simply ask you do do a better job and if you want this in then
please follow his advice.
Asking him to do the job you want to have done is just not the way forward.
Sam
Sam Ravnborg wrote:
> Hi Alain.
>
>> If I may suggest something: what if *YOU* provided a patch (or series of
>> patches, or whatever you like) complying to your high stylistic
>> requirements
>
> hpa does not have any high stylistic requirments he just have good taste.
> He simply ask you do do a better job and if you want this in then
> please follow his advice.
>
> Asking him to do the job you want to have done is just not the way forward.
>
> Sam
Thanks for the advice. You're right, this is not the way forward. I must
admit that I did indeed an huge mistake with this patch.
I've initially developed it a couple of years ago for use in udpcast.
Once done, regular attention was needed for it to adapt it to new
kernels. At one point in time (this September), I figured that I could
save that maintenance effort by getting it integrated into the mainline
kernel. At the same time, I figured, other developers of embedded
systems could profit from it. A win-win situation. Or so I thought.
However, now that I look back on the 3 past months, I figure that the
amount of effort that I poured into this was disproportionate compared
to what I used to spend on maintaining it out of kernel. Even if I
continued evolving it until the end of my life, I would spend _less_
time on it than I did in the last 3 months.
My conclusion is that I'll continue like I did before last summer: to
maintain it on my own website (http://udpcast.linux.lu/source.html)
along with udpcast.
Anybody is still welcome to use it as he sees fit. If a brave soul with
more patience than me wants to plead with the powers-that-be to get it
into official kernel, he has my support. But for myself, I give up on
this quest, it is simply too much hassle to be worthwhile...
Regards,
Alain