Re: [PATCH 00/10] Misc fixes & cleanups for nfs-utils

From: Doug Nazar <nazard@nazar.ca>
To: Steve Dickson <SteveD@RedHat.com>, linux-nfs@vger.kernel.org
Subject: Re: [PATCH 00/10] Misc fixes & cleanups for nfs-utils
Date: Thu, 16 Jul 2020 02:56:49 -0400	[thread overview]
Message-ID: <0256f366-6541-9ae3-3d1b-62f63e6d62c6@nazar.ca> (raw)
In-Reply-To: <c1b8566f-064e-c063-2a6d-94d4bd92709f@RedHat.com>

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