[Ipmitool-devel] ID: 65 - Fixes for configure.in for cross compilation

[Zdenek Styblik]

Ok, let's fork this discussion.

On Wed, Jul 17, 2013 at 4:21 PM, Zdenek Styblik

On Wed, Jul 17, 2013 at 4:06 PM, Zdenek Styblik

I posted 6 patches back on Apr 29, 2013 to fix the issues we had with
removing the -Wno-unused-result flag from CFLAGS that never got
reviewed or acked or anything. This was issue #3608759, now is issue
#65 http://sourceforge.net/p/ipmitool/patches/65/.

As it says in comments - this one particular thing is on hold. Jim
will provide details, if necessary.

I read the comments. It was on hold because of the warnings generated
when the flag was removed. That's why I created the patches to fix
the warnings..

I see. But everything that was on tracker, s1-s9, is committed now. If
these patches weren't attached to any ticket, then yes, they've got
skipped for sure.

yeah I saw that my last patches finally got merged.. thank you..

Strange since you're the one whom created the ticket in the first
place. But SF.net acts in very strange ways.

I think that you can change tickets because you're an Admin. Since
I'm a nobody, I cannot do anything, even touch my own tickets...
sigh...

You're sort of right. They've improved the site and what you say is really true.

Hm. Well, you can send it again to me and I'll see if I have it. But
as I said. If it wasn't attached to any ticket, then I'm sure they've
got skipped. In such case, please create a new ticket and attach these
patches.

ok, I'll send them again.

Nah, don't do that. Send everything you have to me, I'll check what I
got and let you know what/if anything is missing on my drive. It will
less overhead for me, I think.

Now, I've compiled ipmitool-cvs with and without
``-Wno-unused-results'' and I see no difference on STDERR what so
ever.
I see couple warnings I should address, though.

I think ``-Wno-unused-results'' can be removed now. Jim?

No guarantees (at least from me) on commit prior 1.8.13 release
though, as there are still some issues that have to be fixed prior
1.8.13 and they're, well, planned. This, sort of, isn't.

Well it's all part of patch #65... and they were submitted back in
Apr.. and they are fairly simple.
Either way.. It fixes a fairly major regression that breaks builds,
but it's up to you, I guess.
d

Just ... uh ... let's panic later. :)
I mean. If we're talking about the only one thing that's left, as
there is nothing else left than this as far as I know, it's no work.
If we're talking about, say, 10 more patches in need of some sort of
code review and what not, then it would mean some work.
I know we've talked about why it shouldn't be removed now and why it
was put on hold. I'm just lazy to digging through mailing to find out
right now. As I said, there is still time.

Z.

[Zdenek Styblik]

Hello all,

attached are re-works of three functions in 'lib/ipmi_ekanalyzer.c'.
These are based on patches Dan has uploaded/attached to ticket in
question.
Please, somebody do the review, wave the flag, so we can get a move on.

Thanks,
Z.

[Dan Gora]

0001-Re-work-of-ipmi_ek_display_fru_header_detail.patch

This looks ok... It's not necessary to change
ipmi_ek_display_fru_header_detail to return int, especially since you
don't check the return code at all. Also the added storing of the
return code for fseek at the bottom, but not checking the value is
similarly specious, but harmless.

0002-Small-re-work-of-ipmi_ek_display_chassis_info_area.patch

Same with this one.. Totally unnecessary to change the return value to
int since it's just displaying stuff and no one checks the return
code, storing the return value to fseek then not checking it doesn't
change the generated code at all, just adds extra verbiage to the
source, but is harmless.

0003-Small-changes-to-ipmi_ek_display_chassis_info_area.patch

Same with this one..


So the changes to the patch that I submitted look completely
unnecessary, but harmless.


sorry for the original diff format. I have no idea how to drive CVS.

I see you are using git as well.. Is there any chance we can get the
whole tree converted to git and get rid of this CVS stuff? I'm sure
that most people nowadays are learning git, not CVS.


thanks
dan



On Sun, Jul 21, 2013 at 1:44 PM, Zdenek Styblik

Post by Zdenek Styblik
Hello all,
attached are re-works of three functions in 'lib/ipmi_ekanalyzer.c'.
These are based on patches Dan has uploaded/attached to ticket in
question.
Please, somebody do the review, wave the flag, so we can get a move on.
Thanks,
Z.

--
Dan Gora
Software Engineer

Adax, Inc.
Av Dona Maria Alves, 1070 Casa 5
Centro
Ubatuba, SP
CEP 11680-000
Brasil

Tel: [phone] (Brazil and outside of US)
: [phone] (Inside of US)
: dan_gora (Skype)

email: ***@adax.com

[Zdenek Styblik]

Post by Dan Gora
0001-Re-work-of-ipmi_ek_display_fru_header_detail.patch
This looks ok... It's not necessary to change
ipmi_ek_display_fru_header_detail to return int, especially since you
don't check the return code at all. Also the added storing of the
return code for fseek at the bottom, but not checking the value is
similarly specious, but harmless.
0002-Small-re-work-of-ipmi_ek_display_chassis_info_area.patch
Same with this one.. Totally unnecessary to change the return value to
int since it's just displaying stuff and no one checks the return
code, storing the return value to fseek then not checking it doesn't
change the generated code at all, just adds extra verbiage to the
source, but is harmless.
0003-Small-changes-to-ipmi_ek_display_chassis_info_area.patch
Same with this one..

Dan,

I agree that some changes aren't as best as they could be, eg.
checking some return values, and I admit there is still some work
left. I hope code got simpler and compiler shut. Anyone is free to
iterate on these changes and make code better.

Post by Dan Gora
So the changes to the patch that I submitted look completely
unnecessary, but harmless.

Now, I don't know what you mean. I took your changes into account and,
actually, they're there. I just wanted to iterate a bit more.

Post by Dan Gora
sorry for the original diff format. I have no idea how to drive CVS.

No worries. I've applied your patch and then did % cvs diff -u ; to
get unified output. Also, I hope all is in since I've split your patch
into smaller chunks. In retrospect, I could have handled it
differently and perhaps even better - like apply and commit your patch
and then do the re-works. Well, next time.

There are still those(two) scanf() patches left. I want to look at
these closer, because it seems tricky to handle scanf() correctly. I
mean, not just patch it up. I hope we'll be good to remove
-Wno-unused-result after these two.

Post by Dan Gora
I see you are using git as well..

Yes, especially if I want to change multiple things. I just check-in
file/files, do the changes, export diffs.

Post by Dan Gora
Is there any chance we can get the
whole tree converted to git and get rid of this CVS stuff?

I have no idea how to convert CVS to git(here at SF.net). But it's
possible to have git, yes.

Best regards,
Z.

Post by Dan Gora
I'm sure
that most people nowadays are learning git, not CVS.
thanks
dan
On Sun, Jul 21, 2013 at 1:44 PM, Zdenek Styblik

Post by Zdenek Styblik
Hello all,
attached are re-works of three functions in 'lib/ipmi_ekanalyzer.c'.
These are based on patches Dan has uploaded/attached to ticket in
question.
Please, somebody do the review, wave the flag, so we can get a move on.
Thanks,
Z.

--
Dan Gora
Software Engineer
Adax, Inc.
Av Dona Maria Alves, 1070 Casa 5
Centro
Ubatuba, SP
CEP 11680-000
Brasil
Tel: [phone] (Brazil and outside of US)
: [phone] (Inside of US)
: dan_gora (Skype)

[Dan Gora]

On Thu, Jul 25, 2013 at 7:06 AM, Zdenek Styblik

Post by Zdenek Styblik
Dan,
I agree that some changes aren't as best as they could be, eg.
checking some return values, and I admit there is still some work
left. I hope code got simpler and compiler shut. Anyone is free to
iterate on these changes and make code better.

Post by Dan Gora
So the changes to the patch that I submitted look completely
unnecessary, but harmless.

Now, I don't know what you mean. I took your changes into account and,
actually, they're there. I just wanted to iterate a bit more.

What I mean is that the changes that you added to my changes do not
change the generated code. They just add more words to the source
code.

Changing:

fseek(fd, blah, blah);
to
tmp = fseek(fd, blah, blah);

then never looking at 'tmp' will generate the exact same object code
because 'tmp' will just be optimized out. Therefore it is unnecessary
to write the 'tmp =' part. That's why I said that the extra changes
are unnecessary. They do not change anything in the program's
behavior or functionality. If you add the checks of these return
codes, that _will_ change the program's behavior (I personally think
that it's kind of unnecessary, but...).

Post by Zdenek Styblik

Post by Dan Gora
sorry for the original diff format. I have no idea how to drive CVS.

No worries. I've applied your patch and then did % cvs diff -u ; to
get unified output. Also, I hope all is in since I've split your patch
into smaller chunks. In retrospect, I could have handled it
differently and perhaps even better - like apply and commit your patch
and then do the re-works. Well, next time.

It's fine... The changes were all there.

Post by Zdenek Styblik
There are still those(two) scanf() patches left. I want to look at
these closer, because it seems tricky to handle scanf() correctly.

It's really not _that_ tricky. It returns the number of things that
it scanned in correctly.

Post by Zdenek Styblik
I mean, not just patch it up.

Well I didn't just patch it up.. I tried to handle them correctly.

Post by Zdenek Styblik
I hope we'll be good to remove
-Wno-unused-result after these two.

yeah, I'd really like that because, like I said it totally breaks the
compilation on even fairly recent gcc versions.

Post by Zdenek Styblik

Post by Dan Gora
I see you are using git as well..

Yes, especially if I want to change multiple things. I just check-in
file/files, do the changes, export diffs.

Post by Dan Gora
Is there any chance we can get the
whole tree converted to git and get rid of this CVS stuff?

I have no idea how to convert CVS to git(here at SF.net). But it's
possible to have git, yes.

oh man.. that's great to hear. It just so happens there is a tool
'cvs2git' which does a great job of converting the CVS tree to git.
That's what I originally used. We just need a list of contributors so
that we can properly map the email address names used in cvs to real
names.

http://cvs2svn.tigris.org/cvs2git.html

I ran it once and everything looked really good in it. It saved all
the old revisions and tagged all the releases in what looked to me
like the right place.

The other option is to just chose some point and start the git repo
from that point, but it would be nice to have the full change history
if possible.

I'm sure that SF.net supports git..

[Zdenek Styblik]

On Thu, Jul 25, 2013 at 4:28 PM, Dan Gora <***@gmail.com> wrote:
[...]

Post by Dan Gora
What I mean is that the changes that you added to my changes do not
change the generated code. They just add more words to the source
code.
fseek(fd, blah, blah);
to
tmp = fseek(fd, blah, blah);
then never looking at 'tmp' will generate the exact same object code
because 'tmp' will just be optimized out. Therefore it is unnecessary
to write the 'tmp =' part. That's why I said that the extra changes
are unnecessary. They do not change anything in the program's
behavior or functionality. If you add the checks of these return
codes, that _will_ change the program's behavior (I personally think
that it's kind of unnecessary, but...).

I see. Well, somebody should check those return values, but that's a
different story, I guess :)

[...]

Post by Dan Gora

Post by Zdenek Styblik
There are still those(two) scanf() patches left. I want to look at
these closer, because it seems tricky to handle scanf() correctly.

It's really not _that_ tricky. It returns the number of things that
it scanned in correctly.

Post by Zdenek Styblik
I mean, not just patch it up.

Well I didn't just patch it up.. I tried to handle them correctly.

Here is what I had in mind:

http://stackoverflow.com/questions/2430303/disadvantages-of-scanf
http://stackoverflow.com/questions/9457325/how-to-use-sscanf-correctly-and-safely
https://www.securecoding.cert.org/confluence/display/seccode/INT05-C.+Do+not+use+input+functions+to+convert+character+data+if+they+cannot+handle+all+possible+inputs
http://blog.markloiseau.com/2012/02/two-safer-alternatives-to-scanf/

[...]

Post by Dan Gora
oh man.. that's great to hear. It just so happens there is a tool
'cvs2git' which does a great job of converting the CVS tree to git.
That's what I originally used. We just need a list of contributors so
that we can properly map the email address names used in cvs to real
names.
http://cvs2svn.tigris.org/cvs2git.html
I ran it once and everything looked really good in it. It saved all
the old revisions and tagged all the releases in what looked to me
like the right place.
The other option is to just chose some point and start the git repo
from that point, but it would be nice to have the full change history
if possible.
I'm sure that SF.net supports git..

[Dan Gora]

Hi Zdenek,

On Tue, Aug 13, 2013 at 8:17 AM, Zdenek Styblik

Post by Zdenek Styblik

Post by Dan Gora

Post by Zdenek Styblik
There are still those(two) scanf() patches left. I want to look at
these closer, because it seems tricky to handle scanf() correctly.

It's really not _that_ tricky. It returns the number of things that
it scanned in correctly.

Post by Zdenek Styblik
I mean, not just patch it up.

Well I didn't just patch it up.. I tried to handle them correctly.

http://stackoverflow.com/questions/2430303/disadvantages-of-scanf
http://stackoverflow.com/questions/9457325/how-to-use-sscanf-correctly-and-safely
https://www.securecoding.cert.org/confluence/display/seccode/INT05-C.+Do+not+use+input+functions+to+convert+character+data+if+they+cannot+handle+all+possible+inputs
http://blog.markloiseau.com/2012/02/two-safer-alternatives-to-scanf/

My patch made no attempt to perform a security audit of the code. It
was only to get rid of the warnings that were caused by removing the
compilation flag. The same scanf's which were there before are still
there, just the return code is now checked.

If you want to go though all this hassle of getting rid of every scanf
from the code, then that's up to you, but I don't think that there is
any reason to intermingle this fairly massive project with my fairly
simple patch to add return code checking. They are two different
things and should be done in two different patches.

thanks
dan

[Zdenek Styblik]

Post by Dan Gora
My patch made no attempt to perform a security audit of the code. It
was only to get rid of the warnings that were caused by removing the
compilation flag. The same scanf's which were there before are still
there, just the return code is now checked.

Dan,

I know and never said otherwise. And that's where I see problem. This
is not the first attempt like that and I'm afraid once "patched over"
it will be forgotten. As you have said, the same scanf() which were
there before are still there. Put it other and more general way, the
same crap code we had before is still there and we're just patching
over it.
Now, don't take it personally, because that's seems to be pretty much
general idea around here.

Post by Dan Gora
If you want to go though all this hassle of getting rid of every scanf
from the code, then that's up to you, but I don't think that there is
any reason to intermingle this fairly massive project with my fairly
simple patch to add return code checking. They are two different
things and should be done in two different patches.

Very, very sad, but you're right. I did quick grep through sources and
then did double-facepalm. I guess enough said.
I'll give it a commit once I recover.

Z.

[Zdenek Styblik]

On Tue, Aug 13, 2013 at 6:42 PM, Zdenek Styblik
<***@gmail.com> wrote:
[...]

Post by Zdenek Styblik

Post by Dan Gora
If you want to go though all this hassle of getting rid of every scanf
from the code, then that's up to you, but I don't think that there is
any reason to intermingle this fairly massive project with my fairly
simple patch to add return code checking. They are two different
things and should be done in two different patches.

Very, very sad, but you're right. I did quick grep through sources and
then did double-facepalm. I guess enough said.
I'll give it a commit once I recover.
Z.

Dan,

it's in. Removal of '-Wno-unused-result' remains and I hate (new)
SF.net very much.

Regards,
Z.