[OpenAFS-devel] warnings fix

[Marcus Watts]

   Jeffrey Altman <jaltman@secure-endpoints.com> writes:
...
> None of these "interesting surprises" are specific to rxk5.  Maintaining
> them privately and mixing them into the monolithic rxk5 branch makes it
> nearly impossible for them to be incorporated into the tree.
> For each one of these items an RT ticket should have been created
> at the time the issue was discovered.

Please shoot the messenger more.

I had originally planned to do more to split this patch out.  I gave up
when it was clear substantial portions of the patch had already become
obselete, and it was not clear if *any* particular item addressed by the
patch had value.

> 
> Just from the descriptions you have provided below the trivial fix
> could have been recreated without even looking at the source patch.

I'm pleased some of the descriptions were actually useful.

> 
> > /1/
> > src/butc/list.c
> > TapeLog "Warning: Can't write end-of-dump on tape\n"
> > was missing a "0," before the string -- so probably wasn't actually
> > capable of producing the message.
> 
> I think you meant src/butc/dump.c
> 
> http://gerrit.openafs.org/84

Sorry, yes.  Thanks for fixing this.

src/butc/list.c
had a completey different and less interesting problem;
incomplete ansification.  But -- this wasn't an "interesting"
fix -- they're obvious by warning message.  Indeed this particular
file seems to have been fixed.

> 
> > /2/
> > src/auth/cellconfig.c
> > afsconf_GetCellInfo
> > calls lcstring giving it the value acellName -- which means
> > this function can scribble over (lower-case) the input cell name
> > (a "surprise" side-effect).
> 
> I do not believe this is a bug.  When I reviewed this issue several
> years ago I concluded the side-effect was intended because all cell
> names are case-insensitive and the normative representation is in
> lower case.

This started off with noticing that cellname being a "const" in some
functions.  Following that lead to this function.
[	In fact, I dimly recall an unpleasant bug somebody had many
	years back with perhaps just this feature (passing in a kerberos
	4 realm name...)  It was still transarc code at the time, I think... ]
In any case, the input parameter change is not necessary, is not used
anywhere inside openafs, and I think not useful or desirable.  If there
were code that wanted this behavior, it could always call lcstring separately.
Now that I think about it, this will certainly cause pr_Initialize()
in turn to behave differently depending on the string fed into it.
I look forward to seeing documentation on that behavior.

Really, whether this counts as a bug or a feature depends on how it's documented.

> 
> > /3/
> > src/uss/uss_vol.o
> > used vldbentry not nvldbentry.
> > On little-endian machines, MapHostToNetwork won't see the right
> > server count, and may do weird things.
> 
> There are many locations in the source tree where VL_GetEntryByID is
> used instead of attempting to use the newer variants first.
> 
>   src/bucoord/volstub.c
>   src/uss/uss_vol.c
>   src/vlserver/vlclient.c
> 
> RT # 125101

If I remember right, VL_GetEntryByID returns a vldbentry,
MapHostToNetwork takes a nvldbentry.  The data offsets, starting
with the server count, are different, so MapHostToNetwork is going
to do something "funny".  Well, maybe -- depends on machine
architeture & input data.  Past that -- I think I looked at
other MapHostToNetwork uses and decided this was the only case
that looked suspect.

Is a site that lacks the db server support for the newer
variants of VL_* actually likely to use the 1.5.x/cvs head
versions of the uss code?

...
> > /9/
> > src/xstat/xstat_fs_callback.c
> > about line 441 in gator_text_Write
> > debug statements writes out "strToWrite + i" which will write
> > out a character ramp starting at some random character value--the
> > least octet of the address of the string.
> > Should write out "strToWrite[i]"
> 
> I can find no such reference in xstat_fs_callback.c on the master.
> I suspect you meant src/gtx/textobject.c.
> 
> http://gerrit.openafs.org/90

Sorry, probably, yes.

> 
> > /10/
> > src/volser/vos.c
> > ListAddrs,
> > For multihomed addresses, was calling VL_GetAddrsU again
> > passing in &m_uuid, but this is a afsUUID *m_uuid, so this
> > is actually trashing some part of the stack.
> 
> I am unable to find a location in ListAddrs() where m_uuid
> is declared as "afsUUID *".  Please double check this.
> If you do find an issue, please submit a bug to RT or
> a patch to gerrit.

What you are seeing is the result of my trying to be efficient and
comprehensive with very limited time.  If I had approached this the way
you propose, I'd never have gotten this far.  In fact, I probably couldn't
have justified starting on this in the first place.  Sorry, but that's
just the way things are.  You're seeing this because I was checking
to see if any of these items deserved additional time.

In this case, picking 1.5.60 as a convenient reference,
ListAddrs at
src/volser/vos.c:5285
has a local variable "afsUUID m_uuid" and at 
src/volser/vos.c:5370
calls print_addrs(,&m_uuid
which is at
src/volser/vos.c:5191
print_addrs #2 is afsUUID * m_uuid".
at src/volser/vos.c:5229
it calls ubik_VL_GetAddrsU(,,,&m_uuid
so ubik_VL_GetAddrsU is going to receive an afsUUID **.
Since ubik_VL_GetAddrsU is actually expecting a afsUUID*,
it's probably going to stuff an afsUUID into a pointer,
hence some part of ListAddrs's stack gets trashed.

I'm sorry I wasn't more precise about this.

...
> > /12/
> > src/viced/viced.c
> > call to rx_NewServiceHost
> > has nice neatly labelled arguments - scrambled by some
> > automatic code formatter.
> 
> while this is a readability issue it is not a bug.
> I'm not going to change this at the present time just for
> the sake of reformatting the code.

This one showed up here because this particular piece of code
*has* to change for rxk5, also because it's a particularly
glaring example of "automatic code formatter gone wrong".
This was, after all, the "interesting surprises" section,
not the "bugs" list.

So, are you actually asking me to keep "beauty" changes as
a monolithic whole with rxk5 changes?

> 
> ---
> 
> You have heard it from many others in this thread.  Each one
> of the above items could have been submitted as an individual
> bug report to RT and someone would have addressed them.
> If you had included a patch, it would have made it all that
> much easier on the gatekeeper that attempted to close the
> issue.
> 
> Now with the addition of gerrit, submitting patches for issues
> such as these for review, verification and merging is much
> easier.  I hope that in the future you will decide to submit
> patches or bug reports as you come across the issues instead
> of bundling them up as part of a monolithic patch with hundreds
> of unrelated changes.
> 
> Thank you.
> 
> Jeffrey Altman
> 
>

Again, see above.  "comprehensive" is surprisingly the least time
consuming part.  "detailed" and "accurate" makes it a lot more time.
"And we're going to commit patches that address some of your concerns
meanwhile" means the solution didn't converge anymore.  The real question
here was "was there anything useful I found that you didn't find?"
If the answer is yes, arguing I not follow the path that got the useful
stuff is probably not ideal.

					-Marcus