[OpenAFS-devel] warnings fix

[Jeffrey Altman]

Marcus Watts wrote:
> ____ 5. some notable surprises
> 
> openafs.19 also lists 2 more categories that may be of interest,
> "Interesting surprises" and "too complicated to fix here".

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.

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

> /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

> /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.

> /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

> /4/
> ctime usage in printLabel - butc/read_tape.c
> ctime() overwrites results from previous ctime call.
> passing result of ctime()..ctime() to printf will only generate
> repeated results of last ctime call.  which ctime call wins
> is machine/compiler specific.

http://gerrit.openafs.org/85

> /5/
> budb/procs.c
> bogus call to principal_hton
> d.dumper
> budb_principal
>         is a struct -- 256+128+256 character arrays (640 bytes total)
> ktc_principal
>         is a struct -- 64 + 64 + 64 bytes (except on windows where
>                 an extra 256 byte "smbname" appears.) (192 bytes total.)
>
> in the database (on disk)
> struct dump { struct ktc_principal dumper;};
>
> on the wire (rpcs)
> struct budb_dumpEntry { struct budb_principal dumper;};
>
> at about line 1486 (cvs head) in budb/procs.c,
> just before tapeSet_hton in CreateDump()
> have
>         principal_hton(&principal, &d.dumper);
> where principal is a ktc_principal, and d is a "struct dump".
> So here we are using the 640 byte version on a 192 byte field.
> This can instead just be
>         d.dumper = principal

http://gerrit.openafs.org/86

> /6/
> src/bucoord/restore.c
> struct volinfo 1st member: "struct voli".
> warnings only.

http://gerrit.openafs.org/87

> /7/
> src/libadmin/adminutil/afs_utilAdmin.c
> ListCellsRPC
> odd result with call to RXAFSCB_GetCellServDB and
> what is returned in t->cell[slot].serverAddr
> looks like ServerList returned as array of integers?
> If it works at all??

RXAFSCB_GetCellServDB takes a serverList.

http://gerrit.openafs.org/89

> /8/
> src/viced/host.c
> h_DumpHost
> writes out "slot/bit" which is the value of h_holdSlot()
> as a "%d".  For tviced, this was actually a long not an int,
> so this string might come out wrong on some 64 bit machines?

This was fixed by ac3e0ed03187cf7c8af046adb102d6500452815f

> /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

> /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.

> /11/
> src/volser/vsprocs.c
> code called from vos copy | shadow
> UV_CopyVolume2
> this is 1.4.10, 1.5.60 (but not cvs head): about line 2361:
> "Failed to do the%s dump from old site to new site\n",
> -          afromvol);
> +          (flags & RV_NOCLONE) ? "" : " incremental");
> without fix, code would core-dump if it encountered this error.

The openafs-devel-1_5_x branch was merged onto the cvs head
before the conversion to git.  This issue is addressed by
8e0b8243ea8b38e6ceab8f1dc6a0e032d6622568

> /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.

---

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