Re: [Xen-devel] [PATCH 2/2] xen/blkfront: Add BUG_ON to deal withmisbehaving backends.

[Konrad Rzeszutek Wilk]

> > +static const char *op_name(int op)
> > +{
> > +	static const char *names[] = {
> 
> Could/should really be "static const char *const names[]".

It should. I forgot the extra 'const' 
> 
> > +		[BLKIF_OP_READ] = "read",
> > +		[BLKIF_OP_WRITE] = "write",
> > +		[BLKIF_OP_WRITE_BARRIER] = "barrier",
> > +		[BLKIF_OP_FLUSH_DISKCACHE] = "flush",
> > +		[BLKIF_OP_RESERVED_1] = "reserved",
> > +		[BLKIF_OP_DISCARD] = "discard" };
> > +
.. snip..
> > +		if (id >= BLK_RING_SIZE)
> > +			/* We can't safely get the 'struct request' as
> > +			 * the id is busted. So limp along. */
> > +			goto err_out;
> 
> Getting out of the loop here isn't really what I'd call "limp along" -
> it'd likely get the communication to a halt (if there are more
> responses ready). I'd rather see this print something, and then
> continue the loop. Or alternatively, if we really want to stop
> communication in such a case, initiate tear down of the device.
> 
> > +
> >  		req  = info->shadow[id].request;
> >  
> >  		if (bret->operation != BLKIF_OP_DISCARD)
> >  			blkif_completion(&info->shadow[id]);
> >  
> > -		add_id_to_freelist(info, id);
> > +		if (add_id_to_freelist(info, id))
> > +			goto err_out;
> 
> Same here, obviously.

<nods> Will do.
.. snip.k
> > +#define BLKIF_OP_RESERVED_1        4
> 
> If you really want to give this a numeric tag, wouldn't it be better
> to make this _4? But maybe you could leave out the definition here
> altogether, and leave the corresponding names[] slot set to NULL?

That would work too. Please see