viro@parcelfarce.linux.theplanet.co.uk wrote:

> . . .
>
>>+	case BLKROSET: return "set-read-only";
>>+	case BLKROGET: return "get-read-only";
>>+	case BLKGETSIZE: return "block-get-size";
>>+	case BLKFLSBUF: return "flush-buffer-cache";
>>    
>>
>
>The last 4 never make it to the driver (nor should they, being generic
>ioctls).
>  
>
Aarg! A little too fast I was with me cut-and-patse! Actually though, 
BLKFLSBUF and BLKROSET do make it to the driver (at least according to 
my reading of linux-2.5.73/drivers/block/ioctl.c). The others are gone 
in the attached updated patch.

>>+	reply.magic = ntohl(reply.magic);
>>+	if (reply.magic != NBD_REPLY_MAGIC) {
>>+		printk(KERN_ERR "%s: Wrong magic (0x%lx)\n",
>>+				lo->disk->disk_name,
>>+				(unsigned long)reply.magic);
>>    
>>
>
>  
>
>>+	reply.error = ntohl(reply.error);
>>+	if (reply.error) {
>>+		printk(KERN_ERR "%s: Other side returned error (%d)\n",
>>+				lo->disk->disk_name, reply.error);
>>    
>>
>
>Generally a bad taste - it's harmless in this case, but such endianness
>conversions in place are asking for trouble.
>
Sure. I could have used local variables instead to hold the endianess 
results (which seems overkill). Or can just call ntohl again in printk 
and so it is now.

>> 	lo->refcnt--;
>>+	dprintk(DBG_RELEASE, "%s: %s refcnt=%d\n", lo->disk->disk_name,
>>+			__FUNCTION__, lo->refcnt);
>> 	/* N.B. Doesn't lo->file need an fput?? */
>> 	return 0;
>>    
>>
>
>a) please, lose __FUNCTION__ - using it in macros is one thing, but
>directly in a function that got a name shorter than `__FUNCTION__'?
>  
>
Done.

>b) no, it doesn't.
>  
>
The N.B. is a left over from the original which agreeably can be cleaned 
up too. So it's gone now too.

>c) ->refcnt is never used.  How about losing it completely?  Along with
>->open() and ->release()...
>  
>
There's a few other things that could be "lost" too. Like lo->file and 
lo->blksize_bits. I'm trying to go one step at a time though with these 
patches. And actually ->open() and ->release() will be needed later when 
I get that far. ;-)

>> 		sreq.flags = REQ_SPECIAL;
>> 		nbd_cmd(&sreq) = NBD_CMD_DISC;
>>    
>>
>
>;-/
>  
>
That's how it was already. Maybe it's wrong too or just not so good. But 
changing this should wait till a patch that's not dealing with enhanced 
diags no??

>>+		sreq.sector = 0;
>>+		sreq.nr_sectors = 0;
>>    
>>
>
>Umm... What for?
>
I added some comments now to explain why.

>> 	if (sizeof(struct nbd_request) != 28) {
>>-		printk(KERN_CRIT "Sizeof nbd_request needs to be 28 in order to work!\n" );
>>+		printk(KERN_CRIT "nbd: Sizeof nbd_request needs to be 28 in order to work!\n" );
>> 		return -EIO;
>> 	}
>>    
>>
>
>FWIW, I'm less than sure that struct nbd_request is worth defining.
>We use it in two places - the check above and nbd_send_req() where
>it is filled and then its address is cast to char *.  It might be
>better to use u32[7] directly and forget about all alignment issues.
>Hell knows...  In this situation I would probably just define an
>enum for offsets and be done with that.  Same goes for nbd_reply.
>  
>
I've thought about this too. I have to say so far that I don't like the 
idea though of getting rid of these structs. They're shared with the 
remote end also.

Here's the revised patch then for enhanced diagnostics...