Re: [PATCH 2/2] xen/blkback: Make optional features be reallyoptional.

[Jan Beulich]

>>> On 14.03.12 at 19:10, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> They were using the xenbus_dev_fatal() function which would
> change the state of the connection immediately. Which is not
> what we want when we advertise optional features.
> 
> So make 'feature-discard','feature-barrier','feature-flush-cache'
> optional.
> 
> Suggested-by: Jan Beulich <JBeulich@xxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> ---
>  drivers/block/xen-blkback/xenbus.c |   25 ++++++++++---------------
>  1 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/block/xen-blkback/xenbus.c 
> b/drivers/block/xen-blkback/xenbus.c
> index d417c13..d90a39c 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -381,7 +381,7 @@ int xen_blkbk_flush_diskcache(struct xenbus_transaction 
> xbt,
>  	err = xenbus_printf(xbt, dev->nodename, "feature-flush-cache",
>  			    "%d", state);
>  	if (err)
> -		xenbus_dev_fatal(dev, err, "writing feature-flush-cache");
> +		dev_warn(&dev->dev, "writing feature-flush-cache (%d)", err);

I'm really not certain whether dev_warn() or xenbus_dev_error() is
better here. The latter would also make the error visible in xenstore,
but whether that's good, superfluous, or outright bad I'm unsure
about.

>  
>  	return err;
>  }
> @@ -400,16 +400,14 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, 
> struct backend_info *be)
>  			"discard-granularity", "%u",
>  			q->limits.discard_granularity);
>  		if (err) {
> -			xenbus_dev_fatal(dev, err,
> -				"writing discard-granularity");
> +			dev_warn(&dev->dev, "writing discard-granularity (%d)", err);
>  			goto out;
>  		}
>  		err = xenbus_printf(xbt, dev->nodename,
>  			"discard-alignment", "%u",
>  			q->limits.discard_alignment);
>  		if (err) {
> -			xenbus_dev_fatal(dev, err,
> -				"writing discard-alignment");
> +			dev_warn(&dev->dev, "writing discard-alignment (%d)", err);
>  			goto out;
>  		}
>  		state = 1;
> @@ -418,15 +416,14 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, 
> struct backend_info *be)
>  				    "discard-secure", "%d",
>  				    blkif->vbd.discard_secure);
>  		if (err) {
> -			xenbus_dev_fatal(dev, err,
> -					"writting discard-secure");
> +			dev_warn(dev-dev, "writing discard-secure (%d)", err);
>  			goto out;
>  		}
>  	}
>  	err = xenbus_printf(xbt, dev->nodename, "feature-discard",
>  			    "%d", state);
>  	if (err)
> -		xenbus_dev_fatal(dev, err, "writing feature-discard");
> +		dev_warn(&dev->dev, "writing feature-discard (%d)", err);
>  out:
>  	return err;
>  }
> @@ -439,7 +436,7 @@ int xen_blkbk_barrier(struct xenbus_transaction xbt,
>  	err = xenbus_printf(xbt, dev->nodename, "feature-barrier",
>  			    "%d", state);
>  	if (err)
> -		xenbus_dev_fatal(dev, err, "writing feature-barrier");
> +		dev_warn(&dev->dev, "writing feature-barrier (%d)", err);
>  
>  	return err;
>  }
> @@ -671,14 +668,12 @@ again:
>  		return;
>  	}
>  
> -	err = xen_blkbk_flush_diskcache(xbt, be, be->blkif->vbd.flush_support);
> -	if (err)
> -		goto abort;
> +	/* If we can't advertise it is OK. */
> +	(void)xen_blkbk_flush_diskcache(xbt, be, be->blkif->vbd.flush_support);
>  
> -	err = xen_blkbk_discard(xbt, be);
> +	(void)xen_blkbk_discard(xbt, be);
>  
> -	/* If we can't advertise it is OK. */
> -	err = xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
> +	(void)xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);

The casts here are bogus in any case. Even more, the functions can
now easily be declared to return "void", as they don't have a caller
caring for their return value anymore (xen_blkbk_discard() should
even be "static").

Jan

>  
>  	err = xenbus_printf(xbt, dev->nodename, "sectors", "%llu",
>  			    (unsigned long long)vbd_sz(&be->blkif->vbd));


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/