[00/10] Misc fixes & cleanups for nfs-utils
mbox series

Message ID 20200701182803.14947-1-nazard@nazar.ca
Headers show
Series
  • Misc fixes & cleanups for nfs-utils
Related show

Message

Doug Nazar July 1, 2020, 6:27 p.m. UTC
Most of this work centers around gssd, however a few items I did tree
wide. It's been compile tested with both gcc & clang on x86_64 & arm32
and runtime tested on x86_64.


Doug Nazar (10):
  gssd: Refcount struct clnt_info to protect multithread usage
  Update to libevent 2.x apis.
  gssd: Cleanup on exit to support valgrind.
  gssd: gssd_k5_err_msg() returns a strdup'd msg. Use free() to release.
  gssd: Fix locking for machine principal list
  gssd: Add a few debug statements to help track client_info lifetimes.
  gssd: Lookup local hostname when srchost is '*'
  gssd: We never use the nocache param of gssd_check_if_cc_exists()
  Cleanup printf format attribute handling and fix format strings
  Fix various clang warnings.

 aclocal/libevent.m4                |   6 +-
 configure.ac                       |   6 +-
 support/include/compiler.h         |  14 +
 support/include/xcommon.h          |  12 +-
 support/include/xlog.h             |  20 +-
 support/nfs/xcommon.c              |   2 +
 support/nfsidmap/gums.c            |   2 +
 support/nfsidmap/libnfsidmap.c     |   8 +-
 support/nfsidmap/nfsidmap.h        |  10 +-
 support/nfsidmap/nfsidmap_common.c |   1 +
 support/nfsidmap/nss.c             |   4 +-
 support/nfsidmap/regex.c           |   6 +-
 support/nfsidmap/static.c          |   1 +
 support/nfsidmap/umich_ldap.c      |  10 +-
 tools/locktest/testlk.c            |   6 +-
 utils/exportfs/exportfs.c          |   5 +-
 utils/gssd/err_util.h              |   4 +-
 utils/gssd/gss_names.c             |   9 +-
 utils/gssd/gss_util.c              |   2 +-
 utils/gssd/gssd.c                  | 165 ++++++++---
 utils/gssd/gssd.h                  |  10 +-
 utils/gssd/gssd_proc.c             |  14 +-
 utils/gssd/krb5_util.c             | 422 +++++++++++++++++------------
 utils/gssd/krb5_util.h             |  16 +-
 utils/gssd/svcgssd.c               |   4 +-
 utils/gssd/svcgssd_proc.c          |   9 +-
 utils/idmapd/idmapd.c              |  65 +++--
 utils/mount/network.c              |   4 +-
 utils/mount/stropts.c              |   2 -
 utils/mountd/cache.c               |   2 +-
 utils/nfsdcld/cld-internal.h       |   2 +-
 utils/nfsdcld/nfsdcld.c            |  29 +-
 utils/nfsdcld/sqlite.c             |   1 -
 utils/nfsdcltrack/sqlite.c         |   2 +-
 utils/nfsidmap/nfsidmap.c          |   3 +-
 35 files changed, 536 insertions(+), 342 deletions(-)
 create mode 100644 support/include/compiler.h

Comments

Steve Dickson July 14, 2020, 6:38 p.m. UTC | #1
Hey Doug,

On 7/1/20 2:27 PM, Doug Nazar wrote:
> Most of this work centers around gssd, however a few items I did tree
> wide. It's been compile tested with both gcc & clang on x86_64 & arm32
> and runtime tested on x86_64.
> 
> 
> Doug Nazar (10):
>   gssd: Refcount struct clnt_info to protect multithread usage
>   Update to libevent 2.x apis.
>   gssd: Cleanup on exit to support valgrind.
>   gssd: gssd_k5_err_msg() returns a strdup'd msg. Use free() to release.
>   gssd: Fix locking for machine principal list
>   gssd: Add a few debug statements to help track client_info lifetimes.
>   gssd: Lookup local hostname when srchost is '*'
>   gssd: We never use the nocache param of gssd_check_if_cc_exists()
>   Fix various clang warnings.
I did commit all of the above... (tag: nfs-utils-2-5-2-rc1)

I did not commit the following 
   Cleanup printf format attribute handling and fix format strings

because 3 different version were posted 

Cleanup printf format attribute handling and fix various format strings
Cleanup printf format attribute handling and fix format strings
Consolidate printf format attribute handling and fix various format strings

I was not sure which one you wanted and I was wondering what exactly is
being cleaned up? What problems is this solving?

Finally, being this is a whole tree commit and I have a number
of patches in the queue.. I would like to hold off on this one.

A patch like this will cause all those patches in the queue 
not to apply... So once I drain the queue, hopefully you
would not mind rebasing... after we talk about what you 
are trying to do.

I do appreciate the hard work... esp with gssd... I did test
it every step of the way... and it seems to be fairly 
solid... nice work!

steved.
 
> 
>  aclocal/libevent.m4                |   6 +-
>  configure.ac                       |   6 +-
>  support/include/compiler.h         |  14 +
>  support/include/xcommon.h          |  12 +-
>  support/include/xlog.h             |  20 +-
>  support/nfs/xcommon.c              |   2 +
>  support/nfsidmap/gums.c            |   2 +
>  support/nfsidmap/libnfsidmap.c     |   8 +-
>  support/nfsidmap/nfsidmap.h        |  10 +-
>  support/nfsidmap/nfsidmap_common.c |   1 +
>  support/nfsidmap/nss.c             |   4 +-
>  support/nfsidmap/regex.c           |   6 +-
>  support/nfsidmap/static.c          |   1 +
>  support/nfsidmap/umich_ldap.c      |  10 +-
>  tools/locktest/testlk.c            |   6 +-
>  utils/exportfs/exportfs.c          |   5 +-
>  utils/gssd/err_util.h              |   4 +-
>  utils/gssd/gss_names.c             |   9 +-
>  utils/gssd/gss_util.c              |   2 +-
>  utils/gssd/gssd.c                  | 165 ++++++++---
>  utils/gssd/gssd.h                  |  10 +-
>  utils/gssd/gssd_proc.c             |  14 +-
>  utils/gssd/krb5_util.c             | 422 +++++++++++++++++------------
>  utils/gssd/krb5_util.h             |  16 +-
>  utils/gssd/svcgssd.c               |   4 +-
>  utils/gssd/svcgssd_proc.c          |   9 +-
>  utils/idmapd/idmapd.c              |  65 +++--
>  utils/mount/network.c              |   4 +-
>  utils/mount/stropts.c              |   2 -
>  utils/mountd/cache.c               |   2 +-
>  utils/nfsdcld/cld-internal.h       |   2 +-
>  utils/nfsdcld/nfsdcld.c            |  29 +-
>  utils/nfsdcld/sqlite.c             |   1 -
>  utils/nfsdcltrack/sqlite.c         |   2 +-
>  utils/nfsidmap/nfsidmap.c          |   3 +-
>  35 files changed, 536 insertions(+), 342 deletions(-)
>  create mode 100644 support/include/compiler.h
>
Doug Nazar July 16, 2020, 6:56 a.m. UTC | #2
On 2020-07-14 14:38, Steve Dickson wrote:
>
>>    gssd: Refcount struct clnt_info to protect multithread usage
>>    Update to libevent 2.x apis.
>>    gssd: Cleanup on exit to support valgrind.
>>    gssd: gssd_k5_err_msg() returns a strdup'd msg. Use free() to release.
>>    gssd: Fix locking for machine principal list
>>    gssd: Add a few debug statements to help track client_info lifetimes.
>>    gssd: Lookup local hostname when srchost is '*'
>>    gssd: We never use the nocache param of gssd_check_if_cc_exists()
>>    Fix various clang warnings.
> I did commit all of the above... (tag: nfs-utils-2-5-2-rc1)

Oops, I'd been working on an updated patch set. There's nothing really 
actively wrong in the above patches. I'd just gone back and added proper 
NULL checks and error messages for the libevent conversion. I'll rebase 
and send that as an update. Also "gssd: Lookup local hostname when 
srchost is '*'" I think is wrong. After the first day I couldn't get it 
to repeat, and think it was a mis-compile issue. However, the new code 
arrangement is better in that it shows the correct dns name 
transformation. Previously it would use the same buffer for input & 
output which made the log message very confusing. I'll just drop the '*' 
check.

> I did not commit the following
>     Cleanup printf format attribute handling and fix format strings
>
> because 3 different version were posted
>
> Cleanup printf format attribute handling and fix various format strings
> Cleanup printf format attribute handling and fix format strings
> Consolidate printf format attribute handling and fix various format strings
>
> I was not sure which one you wanted and I was wondering what exactly is
> being cleaned up? What problems is this solving?

They're all the same patch. The summary line was wrapping in the cover 
letter so I edited it a few times, not realizing that format-patch was 
creating another file even if I aborted.

So, it actually does a few things, all based around fixing printf style 
formats.

There were 2 different macros defined to add printf format attribute to 
functions, and several open codings. So it first consolidates them into 
one set of macros (although there is a second copy in nfsidmap.h since 
that's an installed file and can't depend on config.h.

Then, there were several functions that were not marked with the printf 
format attribute (nfsidmap plugins and gssd printerr()).

Finally, a cleanup of all the resulting gcc & clang warnings on both 32 
& 64 bit. In several cases some real errors, not enough parameters, 
passing in various types for the dynamic length which requires an int, 
passing in a char** instead of char*, etc. Of course these are mainly 
debugging messages so rarely caused an issue but were in need of 
cleaning up.

> Finally, being this is a whole tree commit and I have a number
> of patches in the queue.. I would like to hold off on this one.
>
> A patch like this will cause all those patches in the queue
> not to apply... So once I drain the queue, hopefully you
> would not mind rebasing... after we talk about what you
> are trying to do.

Not a problem. I have it rebased here and can send it at any time, or 
split it up if you prefer.

> I do appreciate the hard work... esp with gssd... I did test
> it every step of the way... and it seems to be fairly
> solid... nice work!

I've been chasing that threading bug for over a year. Trying to stress 
the number of simultaneous mounts, types, etc. never thinking the issue 
was external. I bet if I went back and correlated the crashes I saw, 
probably happened when I was upgrading or rebooting the kdc.

Doug