From owner-freebsd-scsi@FreeBSD.ORG Thu Jan 18 06:12:37 2007 Return-Path: X-Original-To: freebsd-scsi@freebsd.org Delivered-To: freebsd-scsi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id D2B5416A47E for ; Thu, 18 Jan 2007 06:12:37 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.freebsd.org (Postfix) with ESMTP id 8EDB313C45B for ; Thu, 18 Jan 2007 06:12:37 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from phobos.samsco.home (phobos.samsco.home [192.168.254.11]) (authenticated bits=0) by pooker.samsco.org (8.13.4/8.13.4) with ESMTP id l0I6CS2s078629; Wed, 17 Jan 2007 23:12:33 -0700 (MST) (envelope-from scottl@samsco.org) Message-ID: <45AF0FC6.6000709@samsco.org> Date: Wed, 17 Jan 2007 23:12:22 -0700 From: Scott Long User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.2pre) Gecko/20070111 SeaMonkey/1.1 MIME-Version: 1.0 To: Craig Rodrigues References: <20070118021356.GA9941@crodrigues.org> In-Reply-To: <20070118021356.GA9941@crodrigues.org> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (pooker.samsco.org [168.103.85.57]); Wed, 17 Jan 2007 23:12:33 -0700 (MST) X-Spam-Status: No, score=-1.4 required=3.8 tests=ALL_TRUSTED autolearn=failed version=3.1.1 X-Spam-Checker-Version: SpamAssassin 3.1.1 (2006-03-10) on pooker.samsco.org Cc: freebsd-scsi@freebsd.org Subject: Re: [PATCH] gcc 4.x cleanups of cam code X-BeenThere: freebsd-scsi@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SCSI subsystem List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Jan 2007 06:12:37 -0000 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