#432 PATCH, useless check for fp!=NULL on usb.c code

[Gianfranco Costamagna]

Hi, the following ubuntu patch should be upstreamed to me.
From: Jeremy Kerr jk@ozlabs.org

on src/plugins/usb/usb.c
function:
scsiProbeNew

the check for fp!=NULL is performed on line 126, with a return 1 in case of NULL pointer.

So the check on line 150 seems to be just useless
- if (fp != NULL) {
- fclose(fp);
- fp = NULL;
- }

the proposal is to just do an fclose(fp) here, because it can't be NULL in any case

thanks for considering,
(I can open a pull request if needed)
Gianfranco

[Zdenek Styblik]

Hello,

if() block in question ensures fp gets closed, if not closed already. The if() block you're refering to checks whether fp has been successfully opened.
I'm sorry, but this change doesn't make sense to me as it'd cause file descriptor leak. Or am I missing something?

Thanks,
Z.

[Gianfranco Costamagna]

Hi Zdenek, thanks for the quick answer!
I see now I'm basically asking you to revert 5be090f0474473135a369b85e42442fba0fcbb6f

Lets see:

int
scsiProbeNew(int num_ami_devices, int sg_nos)
{
[snip]
FILE *fp;

fp = fopen("/proc/scsi/sg/device_strs", "r");
if (fp == NULL) {
    /* Return 1 on error */
    return 1;
}

while (1) {

[many nice things, but none of them are actually chaning fp value]
}

*num_ami_devices = numdevfound;
if (fp != NULL) {
    fclose(fp);
    fp = NULL;
}
return 0;

}

the concerns are:
if fp is NULL; it can be only after fopen, and the return 1 makes sure that the subsequent code is not executed.
Since fp is not changed after the fopen, there is no need to check again for the value, but just close it should be fine.
Moreover, having a fp=NULL, for a local variable, and just before a return seems to be a waste of CPU time :)

nobody can see the fp value outside the function, and after setting it to NULL you are returning, what is the point? The idea is to have something like:

fclose(fp);
return 0;

like it was before, but I might be missing something too :)

[Zdenek Styblik]

Moreover, having a fp=NULL, for a local variable, and just before a return seems to be a waste of CPU time :)

I'm sorry, but this is a joke, right?

[Gianfranco Costamagna]

I like jokes, but not when I talk about code ;) can you please explain? I'm not a coder honestly, I just try to maintain software in Debian/Ubuntu.
BTW 1.8.16 is failing for something I don't even understand
https://launchpad.net/ubuntu/+source/ipmitool/1.8.16-1

[Zdenek Styblik]

What you've said previously is correct. 100%. However, there is also a good practice. And if people kept good practice, then we would have far less security issues.
Now, back to the original topic - is there a performance issue hence the worry about wasted CPU time?

As for failing build; I have given a look to build log, but it seems IPMI tool has been build successfully. Even the build log says so. Could it be missing service file that dpkg is complaining about which could potentialy lead to dpkg returning RC > 0?

[Gianfranco Costamagna]

no :) and I don't think it is really a performance issue, not sure how many times the call is performed.
Probably the if inside the infinite loop was worse :)

[Zdenek Styblik]

Since use of implicitly converted pointers is always fatal to the application
on ia64, they are errors. Please correct them for your next upload

I guess this is why the build is marked as failed.

[Gianfranco Costamagna]

Yes, I think the same, even if I never had such issues

"Function index' implicitly converted to pointer at ipmi_sel.c:2397 Functionstrtok_r' implicitly converted to pointer at ipmi_chassis.c:789
Function strdup' implicitly converted to pointer at log.c:112 Functiongetpass' implicitly converted to pointer at ipmi_user.c:486
Function `getpass' implicitly converted to pointer at ipmi_main.c:524"

how to fix them?

[Zdenek Styblik]

What are the CFLAGS? I'm not getting such warnings. But my guess is either proper macros or tmp_pass = (char *)getpass("Password: "); or it might be both or something completely different.

[Gianfranco Costamagna]

/bin/bash ../libtool --silent --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I.. -I../include -Wdate-time -D_FORTIFY_SOURCE=2 -g -O2 -fPIE -fstack-protector-strong -Wformat -Werror=format-security -Wall -Wextra -std=c99 -pedantic -Wformat -Wformat-nonliteral -I /lib/modules/3.13.0-79-generic/build/include -c -o ipmi_sel.lo ipmi_sel.c

ipmi_sel.c:2397:11: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
Function `index' implicitly converted to pointer at ipmi_sel.c:2397
cursor = index((const char *)cursor, ';');

it is better to look at the build log however, it has more information, at the bottom there are the 5 failures, and before the flags and build log.

[Gianfranco Costamagna]

One of them seems to be trivially fixable.

Function `index' implicitly converted to pointer at ipmi_sel.c:2397
--- ipmitool-1.8.16.orig/lib/ipmi_sel.c
+++ ipmitool-1.8.16/lib/ipmi_sel.c
@@ -31,6 +31,7 @@
  */

 #include <string.h>
+#include <strings.h>
 #include <math.h>
 #define __USE_XOPEN /* glibc2 needs this for strptime */
 #include <time.h>

just a missing header file.

[Zdenek Styblik]

I've pushed this one. Makes sense. However, despite copying CFLAGS, or most of them, I don't get these warnings under Debian with the HEAD :\

[Gianfranco Costamagna]

Hi, I did some more work, and actually fixed the failures.
The patch is here
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=816491
(message 31, debdiff)

unfortunately the build still fails on s390x, and Adam Conrad is trying to debug the issue (I'm an Ubuntu Developer, but I don't have access to porter machines)
https://launchpad.net/ubuntu/+source/ipmitool/1.8.16-1ubuntu2
thanks

Gianfranco

[Zdenek Styblik]

Hello,

thank you for spending time with this. I find it to be rather silly and annoying at the same time :( What I really don't understand is why there are only 3 warnings, given one has been fixed by including strings.h, since eg. strdup() is being used all over the place.

Adding -D_GNU_SOURCE to the CFLAGS avoids this problem for me

Yep, that's what I meant by macros.

answer might actually be to drop -std=c99, or to use -std=gnu99

Yes, however C99 conformity of sorts should be desired state of sorts. But that's not problem of distros. Anyway ...

Is there plan to push s/INCLUDES/AM_CPPFLAGS/ fix upstream?

Best regards,
Z.

[Gianfranco Costamagna]

thank you for spending time with this. I find it to be rather silly and annoying at the same time :

actually I like to learn, and each "waste of time", is a great learning experience to me :)

( What I really don't understand is why there are only 3 warnings, given one has >been fixed by including strings.h, since eg. strdup() is being used all over the place.

I don't know, but it should be because such functions are declared after an #ifdef, so probably not using gnu99, but using c99 makes them not defined
please see the irc conversation I had with Ubuntu Developers
http://irclogs.ubuntu.com/2016/03/14/%23ubuntu-devel.txt

Adding -D_GNU_SOURCE to the CFLAGS avoids this problem for me

I'm not sure what does it mean actually, isn't it something like "use gnu99" at the end?

Is there plan to push s/INCLUDES/AM_CPPFLAGS/ fix upstream?

I'm not sure what you mean by this, isn't this upstream? if you mean a pull request I can do it, otherwise feel free to commit yourself :)
thanks!

G.

[Zdenek Styblik]

actually I like to learn, and each "waste of time", is a great learning experience to me :)

Lesson learned on my side as well ;-|

I don't know, but it should be because such functions are declared after an #ifdef, so probably not using gnu99, but using c99 makes them not defined
please see the irc conversation I had with Ubuntu Developer

I see. I will give it a read it tomorrow. Thanks.

I'm not sure what does it mean actually, isn't it something like "use gnu99" at the end?

That's my understading too and that seems to be the only way at the moment as to workaround C99 in a portable(?) way.

if you mean a pull request I can do it

Yes, that's exactly what I meant. Let's give a credit where credit is due.

Thanks,
Z.