Re: [PATCH 4/5] various minor cleanups

[Jim Rees <rees@umich.edu>]

Benny Halevy wrote:

  >> -static char *pretty_sig(char *sig, int siglen)
  >> +static char *pretty_sig(char *sig, uint32_t siglen)
  >>  {
  >>  	static char rs[100];
  >> -	unsigned int i;
  >> +	unsigned long i;
  >>  
  >> -	if (siglen <= 4) {
  >> +	if (siglen <= sizeof i) {
  >>  		memcpy(&i, sig, sizeof i);
  >> -		sprintf(rs, "0x%0x", i);
  >> +		sprintf(rs, "0x%0lx", i);
  > 
  > What about machine endianess?
  > The MDS and clients may be of different gender, no?
  > Also, on 64 bit machines, you may copy 8 bytes while the signature
  > is 4-bytes long so you may copy junk into i.
  > 
  > Benny
  > 
  >>  	} else {
  >> +		if (siglen > sizeof rs - 1)
  >> +			siglen = sizeof rs - 1;
  
  Hmm, for courtesy purposes, how about ending the truncated
  signature with "..."?

I am glad you are paying attention!  I am aware of the shortcomings of
pretty_sig().  In addition to the problems you noted, it also assumes that a
signature over 8 bytes long is representable as a text string, which is not
guaranteed.  The code it replaced was worse.

I put this in because for debugging I need to be able to follow a signature
all the way from my EMC server to the devmapper.  pretty_sig() simply prints
the signature in a way that I can match it up with the signature on the
server.

I don't want to spend a lot of time on this, but I also am uneasy leaving
EMC-specific code in nfs-utils, especially since it can blow up if you use
it against a non-EMC server.  My inclination is to remove this debugging
code when I no longer need it.  I guess at the very least I should put in a
comment.  I am open to suggestions.