[PATCH] gcc 4.x cleanups of cam code

[Scott Long]

Craig Rodrigues wrote:
> Hi,
> 
> Could someone who is more familiar with CAM take a look
> at the pass I did to try to clean up some gcc 4.x compiler warnings?
> gcc 4.x is more intolerant than gcc 3.x of mixing up assignments of
> char * and unsigned char *.
> 
> Thanks.
> 
> 

I'm not a language guru, nor do I care to be.  However, your patch seems
to imply that gcc 4.x requires an increasing amount of casting
obfuscation in otherwise working source code.  Is the point of gcc 4.x
really to merely make C programming even more tedious, or should we be
fixing our interfaces do that they work without all of the clumsy casts
that you are proposing?.  Below are my amateur comments:

Index: cam.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/cam.c,v
retrieving revision 1.10
diff -u -u -r1.10 cam.c
--- cam.c	18 Apr 2006 21:53:39 -0000	1.10
+++ cam.c	18 Jan 2007 02:05:36 -0000
@@ -104,7 +104,7 @@
  #endif

  void
-cam_strvis(u_int8_t *dst, const u_int8_t *src, int srclen, int dstlen)
+cam_strvis(char *dst, const char *src, int srclen, int dstlen)
  {

  	/* Trim leading/trailing spaces, nulls. */
@@ -115,9 +115,9 @@
  		srclen--;

  	while (srclen > 0 && dstlen > 1) {
-		u_int8_t *cur_pos = dst;
+		char *cur_pos = dst;

-		if (*src < 0x20 || *src >= 0x80) {
+		if ((u_char)*src < 0x20 || (u_char)*src >= 0x80) {
  			/* SCSI-II Specifies that these should never occur. */
  			/* non-printable character */
  			if (dstlen > 4) {

You've gone from an unsigned quantity to a signed quantity.  On the down 
side, this breaks the old API, though in a fairly trivial way. On the 
plus side, it brings the function more in line with the standard 
strvis(3) function.  Not sure exactly how I feel about this.  Since 
cam.c and cam.h are shared with userland, have you verified that this
works there too?

@@ -147,7 +147,7 @@
   * wildcard '?' matches a single non-space character.
   */
  int
-cam_strmatch(const u_int8_t *str, const u_int8_t *pattern, int str_len)
+cam_strmatch(const char *str, const char *pattern, int str_len)
  {

  	while (*pattern != '\0'&& str_len > 0) {

Same as above.

Index: cam.h
===================================================================
RCS file: /home/ncvs/src/sys/cam/cam.h,v
retrieving revision 1.11
diff -u -u -r1.11 cam.h
--- cam.h	5 Jan 2005 22:34:34 -0000	1.11
+++ cam.h	18 Jan 2007 02:05:36 -0000
@@ -199,9 +199,9 @@
  caddr_t	cam_quirkmatch(caddr_t target, caddr_t quirk_table, int 
num_entries,
  		       int entry_size, cam_quirkmatch_t *comp_func);

-void	cam_strvis(u_int8_t *dst, const u_int8_t *src, int srclen, int 
dstlen);
+void	cam_strvis(char *dst, const char *src, int srclen, int dstlen);

-int	cam_strmatch(const u_int8_t *str, const u_int8_t *pattern, int 
str_len);
+int	cam_strmatch(const char *str, const char *pattern, int str_len);
  const struct cam_status_entry*
  	cam_fetch_status_entry(cam_status status);
  #ifdef _KERNEL

Again, if you're going to go forward with this kind of change, please 
make sure that at least camcontrol still compiles.

Index: cam_periph.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/cam_periph.c,v
retrieving revision 1.64
diff -u -u -r1.64 cam_periph.c
--- cam_periph.c	5 Dec 2006 07:45:27 -0000	1.64
+++ cam_periph.c	18 Jan 2007 02:05:36 -0000
@@ -648,7 +648,7 @@
  		mapinfo->bp[i]->b_saveaddr = mapinfo->bp[i]->b_data;

  		/* put our pointer in the data slot */
-		mapinfo->bp[i]->b_data = *data_ptrs[i];
+		mapinfo->bp[i]->b_data = (caddr_t)*data_ptrs[i];

  		/* set the transfer length, we know it's < DFLTPHYS */
  		mapinfo->bp[i]->b_bufsize = lengths[i];
@@ -676,7 +676,7 @@
  		}

  		/* set our pointer to the new mapped area */
-		*data_ptrs[i] = mapinfo->bp[i]->b_data;
+		*data_ptrs[i] = (u_int8_t *)mapinfo->bp[i]->b_data;

  		mapinfo->num_bufs_used++;
  	}

I've seen this cast in other patches that have been pushed out for gcc
4.x.  It seems like every single assignment involving bp->b_data in the
kernel is going to need a clumsy cast from now on.  How thrilling.  I
wonder if there is a better way to do this.  Also, I thought that the
use of caddr_t had been frowned upon many years ago.

Index: scsi/scsi_cd.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/scsi/scsi_cd.c,v
retrieving revision 1.97
diff -u -u -r1.97 scsi_cd.c
--- scsi/scsi_cd.c	5 Dec 2006 07:45:27 -0000	1.97
+++ scsi/scsi_cd.c	18 Jan 2007 02:05:36 -0000
@@ -1522,7 +1522,7 @@
  					/* lba */ bp->bio_offset /
  					  softc->params.blksize,
  					bp->bio_bcount / softc->params.blksize,
-					/* data_ptr */ bp->bio_data,
+					/* data_ptr */(u_int8_t *)bp->bio_data,
  					/* dxfer_len */ bp->bio_bcount,
  					/* sense_len */ SSD_FULL_SIZE,
  					/* timeout */ 30000);

More b_data fun.  I wonder what /sys/kern and /sys/geom look like =-(

I won't bother pasting in all of the scsi_low.[ch] changes, but I assume
that their API change has been confirmed to not any drivers that rely on
them, yes?

Index: scsi/scsi_ses.c
===================================================================
RCS file: /home/ncvs/src/sys/cam/scsi/scsi_ses.c,v
retrieving revision 1.33
diff -u -u -r1.33 scsi_ses.c
--- scsi/scsi_ses.c	5 Dec 2006 07:45:28 -0000	1.33
+++ scsi/scsi_ses.c	18 Jan 2007 02:05:36 -0000
@@ -676,7 +676,7 @@
  	}

  	ccb = cam_periph_getccb(ssc->periph, 1);
-	cam_fill_csio(&ccb->csio, 0, sesdone, ddf, MSG_SIMPLE_Q_TAG, dptr,
+	cam_fill_csio(&ccb->csio, 0, sesdone, ddf, MSG_SIMPLE_Q_TAG, (u_int8_t 
*)dptr,
  	    dlen, sizeof (struct scsi_sense_data), cdbl, 60 * 1000);
  	bcopy(cdb, ccb->csio.cdb_io.cdb_bytes, cdbl);


Blah, another ugly cast.  Avoiding the cast looks to require a lot of 
work, but I don't know if it's ultimately the right thing.

@@ -728,7 +728,7 @@
  static enctyp
  ses_type(void *buf, int buflen)
  {
-	unsigned char *iqd = buf;
+	char *iqd = buf;

  	if (buflen < 8+SEN_ID_LEN)
  		return (SES_NONE);
@@ -762,7 +762,7 @@
  		return (SES_NONE);
  	}

-	if (STRNCMP((char *)&iqd[SAFTE_START], "SAF-TE", SAFTE_LEN - 2) == 0) {
+	if (STRNCMP(&iqd[SAFTE_START], "SAF-TE", SAFTE_LEN - 2) == 0) {
  		return (SES_SAFT);
  	}
  	return (SES_NONE);


Finally, a cast is removed!

Scott