[Debburn-devel] [Debburn-changes] r787 - in cdrkit/trunk: genisoimage genisoimage/diag icedax include libhfs_iso librols libusal libusal/usal wodim

[Harald Hoyer]

Peter Samuelson wrote:
> [harald-hoyer-guest at alioth.debian.org]
>> --- cdrkit/trunk/genisoimage/diag/dump.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/genisoimage/diag/dump.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -366,9 +366,10 @@
>>  	on_comerr((void(*)(int, void *))reset_tty, NULL);
>>  
>>  	do {
>> +	        int ret;
>>  		if (file_addr < (off_t)0) file_addr = (off_t)0;
>>  		showblock(1);
>> -		read(STDIN_FILENO, &c, 1);
>> +		ret = read(STDIN_FILENO, &c, 1);
>>  		if (c == 'a')
>>  			file_addr -= PAGE;
>>  		if (c == 'b')
> 
> I _really_ don't like this.  If we need to tell gcc not to warn us
> about lack of error handling, we should use (void) rather than making
> the code _look_ like we are handling errors when in reality we are not.
> And please add a comment saying why error checking doesn't matter here.
> 
> (Also, read() returns ssize_t, not int.)

will revert.

> 
> Same goes for lots of other instances of the same thing.
> 
>> --- cdrkit/trunk/genisoimage/jte.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/genisoimage/jte.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -535,13 +535,13 @@
>>      }
>>  
>>      memset(buf, 0, sizeof(buf));
>> -    while (fgets(buf, sizeof(buf), md5_file))
>> +    while (fgets((char *)buf, sizeof(buf), md5_file))
> 
> This sort of thing just makes me wonder: why is 'buf' not already
> defined as a char* or void* ?  Shouldn't that be fixed instead?

maybe

> 
>> --- cdrkit/trunk/librols/fexec.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/librols/fexec.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -204,6 +204,8 @@
>>  	int	o[3];
>>  	int	f[3];
>>  	int	errsav;
>> +
>> +	o[0] = o[1] = o[2] = f[0] = f[1] = f[2] = 0;
> 
>       int o[] = { 0, 0, 0 }, f[] = { 0, 0, 0 };    seems simpler.

make it so...

> 
> 
>> --- cdrkit/trunk/librols/raisecond.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/librols/raisecond.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -58,8 +58,8 @@
>>  #ifndef	STDERR_FILENO
>>  #define	STDERR_FILENO	2
>>  #endif
>> -#define	eprints(a)	(void)write(STDERR_FILENO, (a), sizeof (a)-1)
>> -#define	eprintl(a)	(void)write(STDERR_FILENO, (a), strlen(a))
>> +#define	eprints(a)	do { int ret; ret = write(STDERR_FILENO, (a), sizeof (a)-1); } while (0)
>> +#define	eprintl(a)	do { int ret; ret = write(STDERR_FILENO, (a), strlen(a)); } while (0)
> 
> What is the purpose of this change?

throw away the return value. newer gcc checks ignore (void).

> 
>> --- cdrkit/trunk/libusal/scsi-linux-sg.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/libusal/scsi-linux-sg.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -181,7 +181,7 @@
>>  struct usal_local {
>>  	int	usalfile;		/* Used for SG_GET_BUFSIZE ioctl()*/
>>  	short	usalfiles[MAX_SCG][MAX_TGT][MAX_LUN];
>> -  char *filenames[MAX_SCG][MAX_TGT][MAX_LUN];
>> +        char    *filenames[MAX_SCG][MAX_TGT][MAX_LUN];
> 
> What is the purpose of this change?
> 
>> @@ -594,13 +596,14 @@
>>  
>>  			/* that's crap, should not be reached in non-scan mode.
>>  			 * Let's see whether it can be mapped to an atapi
>> -			 * device to emulate some old cludge's behaviour. */
>> +			 * device to emulate some old cludge's behaviour. 
>>  			if(!in_scanmode && busno < 1000 && busno >=0) {
>>  				fake_atabus=1;
>>  				fprintf(stderr, "Unable to open this SCSI ID. Trying to map to old ATA syntax."
>>  						"This workaround will disappear in the near future. Fix your configuration.");
>>  				goto retry_scan_open;
>>  			}
>> +			*/
> 
> This doesn't look like it has anything to do with gcc warnings.

yes... sorry. This prevents an infinite loop.

> 
>> @@ -801,7 +804,7 @@
>>  	for (i = 0; i < 1000; i++) {	/* Read at least 32k from /dev/sg* */
>>  		int	ret;
>>  
>> -		ret = read(f, &sg_rep, sizeof (sg_rep));
>> +		ret = read(f, &sg_rep, sizeof (struct sg_rep));
> 
> What is the purpose of this change?  sizeof(variable) is generally
> better than sizeof(type), because it stays correct if the type changes.

correct, something fishy about gcc made me make this change.

> 
>> --- cdrkit/trunk/libusal/scsierrs.c	2008-01-30 20:50:51 UTC (rev 786)
>> +++ cdrkit/trunk/libusal/scsierrs.c	2008-02-25 11:14:07 UTC (rev 787)
>> @@ -55,6 +55,7 @@
>>  const char	*usal_sensemsg(int, int, int, const char **, char *, int maxcnt);
>>  int usal__errmsg(SCSI *usalp, char *obuf, int maxcnt, struct scsi_sense *, 
>>  					 struct scsi_status *, int);
>> +#ifdef ABC
>>  /*
>>   * Map old non extended sense to sense key.
>>   */
>> @@ -67,6 +68,7 @@
>>  	6, 6, 6, 5,  4,11,11,11			/* 0x28-0x2f */
>>  };
>>  #define	MAX_ADAPTEC_KEYS (sizeof (sd_adaptec_keys))
>> +#endif
> 
> The more usual way to comment out a large block of code is "#if 0"

Well, look at the source:
$ fgrep ifdef libusal/scsierrs.c
#ifdef ABC
#ifdef  __used__
#ifdef  __used__
#ifdef  __used__
#ifdef  XXX
#ifdef  XXX
#ifdef  __used__
#ifdef  __used__
#ifdef  __used__
#ifdef  XXX
#ifdef  __used__
#ifdef  XXX
#ifdef SMO_C501
#ifdef ABC
#ifdef  SMO_C501
#ifdef  DEBUG
#ifdef  SMO_C501
#ifdef  CDD_521
#ifdef ACB

seems like we have to cleanup anyway...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3623 bytes
Desc: S/MIME Cryptographic Signature
Url : http://lists.alioth.debian.org/pipermail/debburn-devel/attachments/20080226/302d7529/attachment.bin