Re: [Xen-devel] [PATCH] xen-blkfront: remove type check from blkfront_setup_discard

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/10/2014 11:28 AM, Olaf Hering wrote:
> In its initial implementation a check for "type" was added, but only phy
> and file are handled. This breaks advertised discard support for other
> type values such as qdisk.
>
> Fix and simplify this function: If the backend advertises discard
> support it is supposed to implement it properly, so enable
> feature_discard unconditionally. If the backend advertises the need for
> a certain granularity and alignment then propagate both properties to
> the blocklayer. The discard-secure property is a boolean, update the code
> to reflect that.
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> ---
>   drivers/block/xen-blkfront.c | 40 ++++++++++++++--------------------------
>   1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index c4a4c90..c9e96b9 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1635,36 +1635,24 @@ blkfront_closing(struct blkfront_info *info)
>   static void blkfront_setup_discard(struct blkfront_info *info)
>   {
>         int err;
> -       char *type;
>         unsigned int discard_granularity;
>         unsigned int discard_alignment;
>         unsigned int discard_secure;
>   
> -	type = xenbus_read(XBT_NIL, info->xbdev->otherend, "type", NULL);
> -       if (IS_ERR(type))
> -               return;
> -
> -       info->feature_secdiscard = 0;
> -       if (strncmp(type, "phy", 3) == 0) {
> -               err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> -                       "discard-granularity", "%u", &discard_granularity,
> -                       "discard-alignment", "%u", &discard_alignment,
> -                       NULL);
> -               if (!err) {
> -                       info->feature_discard = 1;
> -                       info->discard_granularity = discard_granularity;
> -                       info->discard_alignment = discard_alignment;
> -               }
> -               err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> -                           "discard-secure", "%d", &discard_secure,
> -                           NULL);
> -               if (!err)
> -                       info->feature_secdiscard = discard_secure;
> -
> -       } else if (strncmp(type, "file", 4) == 0)
> -               info->feature_discard = 1;
> -
> -       kfree(type);
> +       info->feature_discard = 1;
If the call below fails, is it safe to continue using discard feature? 
At the least, are discard_granularity and discard_alignment guaranteed 
to have sane/safe values?
> +       err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +               "discard-granularity", "%u", &discard_granularity,
> +               "discard-alignment", "%u", &discard_alignment,
> +               NULL);
> +       if (!err) {
> +               info->discard_granularity = discard_granularity;
> +               info->discard_alignment = discard_alignment;
> +       }
> +       err = xenbus_gather(XBT_NIL, info->xbdev->otherend,
> +                   "discard-secure", "%d", &discard_secure,
> +                   NULL);
> +       if (!err)
> +               info->feature_secdiscard = !!discard_secure;
>   }
err variable is not really necessary so you can drop it.


-boris
>   
>   static int blkfront_setup_indirect(struct blkfront_info *info)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel