Re: [PATCH 2/2] blkdiscard: fail on sector misalignment

["Lukáš Czerner" <lczerner@redhat.com>]

On Thu, 23 Oct 2014, Federico Simoncelli wrote:

> Date: Thu, 23 Oct 2014 15:34:36 +0000
> From: Federico Simoncelli <fsimonce@redhat.com>
> To: util-linux@vger.kernel.org
> Cc: kzak@redhat.com, lczerner@redhat.com,
>     Federico Simoncelli <fsimonce@redhat.com>
> Subject: [PATCH 2/2] blkdiscard: fail on sector misalignment
> 
> ---
>  sys-utils/blkdiscard.c            | 10 +++++++---
>  tests/expected/blkdiscard/offsets | 12 ++++++------
>  2 files changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/sys-utils/blkdiscard.c b/sys-utils/blkdiscard.c
> index 1eb2b285..76c72b8 100644
> --- a/sys-utils/blkdiscard.c
> +++ b/sys-utils/blkdiscard.c
> @@ -144,9 +144,9 @@ int main(int argc, char **argv)
>  	if (ioctl(fd, BLKSSZGET, &secsize))
>  		err(EXIT_FAILURE, _("%s: BLKSSZGET ioctl failed"), path);
>  
> -	/* align range to the sector size */
> -	range[0] = (range[0] + secsize - 1) & ~(secsize - 1);
> -	range[1] &= ~(secsize - 1);
> +	/* check offset alignment to the sector size */
> +	if (range[0] % secsize)
> +		errx(EXIT_FAILURE, _("%s: offset %" PRIu64 " is not aligned"), path, range[0]);
>  
>  	/* is the range end behind the end of the device ?*/
>  	if (range[0] > blksize)
> @@ -155,6 +155,10 @@ int main(int argc, char **argv)
>  	if (end < range[0] || end > blksize)
>  		range[1] = blksize - range[0];
>  
> +	/* check length alignment to the sector size */
> +	if (range[1] % secsize)
> +		errx(EXIT_FAILURE, _("%s: length %" PRIu64 " is not aligned"), path, range[1]);
> +

Looks good, however remember that the tool is intended to be used by
the end user, can we be a bit more helpful ? We already know the
secsize it should be aligned to, so we can print that as well.

However it just occurred to me that aligning it to secsize is not
enough. It should be aligned to discard_granularity.

Thanks!
-Lukas

>  	if (secure) {
>  		if (ioctl(fd, BLKSECDISCARD, &range))
>  			err(EXIT_FAILURE, _("%s: BLKSECDISCARD ioctl failed"), path);
> diff --git a/tests/expected/blkdiscard/offsets b/tests/expected/blkdiscard/offsets
> index 766c39d..abedb08 100644
> --- a/tests/expected/blkdiscard/offsets
> +++ b/tests/expected/blkdiscard/offsets
> @@ -1,16 +1,16 @@
>  create loop device from image
>  testing offsets with full block size
>  Discarded 10485760 bytes from the offset 0
> -Discarded 10485248 bytes from the offset 512
> -Discarded 10485248 bytes from the offset 512
> +blkdiscard: offset 1 is not aligned
> +blkdiscard: offset 511 is not aligned
>  Discarded 10485248 bytes from the offset 512
>  Discarded 10484736 bytes from the offset 1024
>  testing offsets with specific length
>  Discarded 5242880 bytes from the offset 0
> -Discarded 5242880 bytes from the offset 0
> -Discarded 5242880 bytes from the offset 0
> -Discarded 5242880 bytes from the offset 512
> -Discarded 5242880 bytes from the offset 512
> +blkdiscard: length 5242881 is not aligned
> +blkdiscard: length 5243391 is not aligned
> +blkdiscard: offset 1 is not aligned
> +blkdiscard: offset 511 is not aligned
>  Discarded 5242880 bytes from the offset 512
>  Discarded 5242880 bytes from the offset 1024
>  detach loop device from image
>