Various non-critical fixes #208

masatake · 2022-04-29T02:33:33Z

Close #206.

I introduced -Wall option in this pull request.
As the result, we'll get more warnings.

That did not work how I wanted it to work :(

masatake · 2022-04-29T04:33:07Z

That did not work how I wanted it to work :(

???
Which commit are you talking about?

print.c

arg.c

dilinger · 2022-04-29T05:08:35Z

That did not work how I wanted it to work :(

??? Which commit are you talking about?

I was hoping that I could approve commits individually, but instead by clicking "approve" it approved the entire PR.

Other than the fill_portmap thing, it all looks fine.

dilinger · 2022-04-29T05:24:29Z

#211 for my suggested fill_portmap change.

BenBE · 2022-04-29T07:18:06Z

dialects/linux/Makefile

@@ -18,7 +18,7 @@ CDEF=
 CDEFS=  ${CDEF} ${CFGF}
 DEP=	${CFGD} ${CFGDN}
 INCL=	${DINC}
-CFLAGS=	${CDEFS} ${INCL} ${DEP} ${DEBUG}
+CFLAGS=	-Wall ${CDEFS} ${INCL} ${DEP} ${DEBUG}


Why not put this in the (currently empty) CDEF assignment below, which propagates to here anyway?

I guess the original author introduced CDEF for allowing a user to add extra definition flags (-D) from the command line of make. So I didn't put '-Wall' to CDEF. Just guessing.

The way those variables are handled right now they always override anything provided on the command line. Would have to be CDEFS?= to allow overriding by the user.

BenBE · 2022-04-29T07:21:30Z

dialects/linux/dsock.c

 #define TCPUDP_IPC_HASH(tp) ((int)(((((tp)->faddr			\
-				      + (tp)->laddr,			\
+				      + (tp)->laddr			\
 				      + (tp)->fport			\
 				      + (tp)->lport			\
 				      + (tp)->proto) * 31415) >> 3)	\


Should use distinct prime factors (large, distinct) to minimize number of collisions. Currently this hash function has obvious collisions that can occur easily.

Thank you.
However, in this pull request, I would like to focus on suppressing the warnings reported by the I compiler.
So, could you make a pull request after I merged this pull request?

Can try to prepare one.

BenBE · 2022-04-29T07:22:22Z

dialects/linux/dsock.c

 #define TCPUDP6_IPC_HASH(tp) ((int)((((TCPUDP6_IPC_ADDR_MK_INT(&(tp)->faddr) \
-				       + TCPUDP6_IPC_ADDR_MK_INT(&(tp)->laddr), \
+				       + TCPUDP6_IPC_ADDR_MK_INT(&(tp)->laddr) \
 				       + (tp)->fport			\
 				       + (tp)->lport			\
 				       + (tp)->proto) * 31415) >> 3)	\


Same remark as with the above UDP hash.

Thank you.
However, in this pull request, I would like to focus on suppressing the warnings reported by the I compiler.
So, could you make a pull request after I merged this pull request?

masatake · 2022-04-29T08:57:27Z

@dilinger,

#211 for my suggested fill_portmap change.

You know what I want!
Allow me to cherry-pick your change so we can close #206 with this pull request.

@kloczek

bitwise & was used mistakenly. Fortunately, the mistake and this fix have no impact on output of lsof. Inspired by the issue lsof-org#206 reported by Tomasz Kłoczko (@kloczek). Signed-off-by: Masatake YAMATO <yamato@redhat.com>

@kloczek

There were typos in the code calculating hash values. The typos might break the flatness of hashtables where the endpoint information about locally used tcp/udp was stored. Theoretically, this fix may improve the performance of lsof with [+|-]E option. Inspired by the issue lsof-org#206 reported by Tomasz Kłoczko (@kloczek). Signed-off-by: Masatake YAMATO <yamato@redhat.com>

@kloczek

Partially close lsof-org#206 reported by @kloczek. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

@kloczek

structure variables, "dummy_utmp" was defined just for getting the size of its member. It was not needed in run-time. Partially close lsof-org#206 reported by @kloczek. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Signed-off-by: Masatake YAMATO <yamato@redhat.com>

File static variables, "copyright", were defined multiple places. They were not referenced anywhere in lsof source code. It seems that the definitions were for embedding copyright notice to object files. However, modern build-systems eliminate unused text from object files. Therefore, the texts defined with "copyright" variables have no expected effect. Furthermore, they are harmful; they cause "unused variables" warnings reported by compilers as reported in lsof-org#206. Partially close lsof-org#206. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

This fix may not have any impact at run-time because the value is never referenced. This change is just for suppressing a compiler warning. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

The pm variable is only used if !defined(HASNORPC_H), so we should protect it with an #if. However, rather than doing that let's move it to the fill_portmap function itself. One less #if to mentally process.

Signed-off-by: Masatake YAMATO <yamato@redhat.com>

BenBE

Changes LGTM (sans hash function as per discussion in comments, PR will follow).

masatake · 2022-04-29T13:41:10Z

@BenBE, thank you for reviewing.

dilinger

Looks good!

masatake · 2022-04-29T16:50:06Z

Thank you for revieiwng.

masatake requested a review from dilinger April 29, 2022 03:08

dilinger previously approved these changes Apr 29, 2022

View reviewed changes

dilinger reviewed Apr 29, 2022

View reviewed changes

print.c Outdated Show resolved Hide resolved

dilinger reviewed Apr 29, 2022

View reviewed changes

arg.c Outdated Show resolved Hide resolved

BenBE reviewed Apr 29, 2022

View reviewed changes

masatake and others added 9 commits April 29, 2022 18:12

Replace & operator with && operator

e4c4386

bitwise & was used mistakenly. Fortunately, the mistake and this fix have no impact on output of lsof. Inspired by the issue lsof-org#206 reported by Tomasz Kłoczko (@kloczek). Signed-off-by: Masatake YAMATO <yamato@redhat.com>

[linux] delete unused variables

6032647

Partially close lsof-org#206 reported by @kloczek. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Delete unused variables

632bf60

structure variables, "dummy_utmp" was defined just for getting the size of its member. It was not needed in run-time. Partially close lsof-org#206 reported by @kloczek. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Show copyright notice in --version output

3ce2f8e

Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Initialize an uninitialize local variable

de7a126

This fix may not have any impact at run-time because the value is never referenced. This change is just for suppressing a compiler warning. Signed-off-by: Masatake YAMATO <yamato@redhat.com>

Clean up the call to fill_portmap

96eea52

The pm variable is only used if !defined(HASNORPC_H), so we should protect it with an #if. However, rather than doing that let's move it to the fill_portmap function itself. One less #if to mentally process.

[linux] compile with -Wall option

0eb078a

Signed-off-by: Masatake YAMATO <yamato@redhat.com>

masatake force-pushed the fix-206 branch from 7f79cd5 to 0eb078a Compare April 29, 2022 10:15

BenBE approved these changes Apr 29, 2022

View reviewed changes

dilinger approved these changes Apr 29, 2022

View reviewed changes

masatake merged commit 84d159d into lsof-org:master Apr 29, 2022

masatake mentioned this pull request Apr 29, 2022

Clean up the call to fill_portmap #211

Closed

BenBE mentioned this pull request Apr 30, 2022

[linux] Avoid some easy collissions for udp/udp6 sockets when hashing #212

Merged

masatake mentioned this pull request Apr 30, 2022

[linux] pass -Wall to CFLAGS by default #207

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Various non-critical fixes #208

Various non-critical fixes #208

masatake commented Apr 29, 2022

masatake commented Apr 29, 2022

dilinger commented Apr 29, 2022

dilinger commented Apr 29, 2022

BenBE Apr 29, 2022

masatake Apr 29, 2022

BenBE Apr 29, 2022

BenBE Apr 29, 2022

masatake Apr 29, 2022

BenBE Apr 29, 2022

BenBE Apr 29, 2022

masatake Apr 29, 2022

masatake commented Apr 29, 2022 •

edited

Loading

BenBE left a comment

masatake commented Apr 29, 2022

dilinger left a comment

masatake commented Apr 29, 2022

Various non-critical fixes #208

Various non-critical fixes #208

Conversation

masatake commented Apr 29, 2022

masatake commented Apr 29, 2022

dilinger commented Apr 29, 2022

dilinger commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masatake commented Apr 29, 2022 • edited Loading

BenBE left a comment

Choose a reason for hiding this comment

masatake commented Apr 29, 2022

dilinger left a comment

Choose a reason for hiding this comment

masatake commented Apr 29, 2022

masatake commented Apr 29, 2022 •

edited

Loading