Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
Suggest function comment /** * Return logical block size, or zero if we can't figure it out */ { -BDRVRawState *s = bs-opaque; -char *buf; -unsigned int sector_size; - -/* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ -if (bs-sg || !s-needs_alignment) { -bs-request_alignment = 1; -s-buf_align = 1; -return; -} +unsigned int sector_size = 0; Pointless initialization. If I do not initialize the sector_size, and the ioctl fails, I will return garbage as a blocksize to the caller. /* Try a few ioctls to get the right size */ -bs-request_alignment = 0; -s-buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef CONFIG_XFS if (s-is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, da) = 0) { -bs-request_alignment = da.d_miniosz; +sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s-buf_align = da.d_mem; */ Since you keep the enabled assignments to s-buf_align out of this function, you should keep out this disabled one, too. +return sector_size; } } #endif +return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. +{ +unsigned int blk_size = 0; Pointless initialization. Same here. +#ifdef BLKPBSZGET +if (ioctl(fd, BLKPBSZGET, blk_size) = 0) { +return blk_size; +} +#endif + +return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ +BDRVRawState *s = bs-opaque; +char *buf; + +/* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ +if (bs-sg || !s-needs_alignment) { +bs-request_alignment = 1; +s-buf_align = 1; +return; +} + +s-buf_align = 0; +/* Let's try to use the logical blocksize for the alignment. */ +bs-request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s-buf_align) { size_t align;
Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
Ekaterina Tumanova tuman...@linux.vnet.ibm.com writes: Suggest function comment /** * Return logical block size, or zero if we can't figure it out */ { -BDRVRawState *s = bs-opaque; -char *buf; -unsigned int sector_size; - -/* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ -if (bs-sg || !s-needs_alignment) { -bs-request_alignment = 1; -s-buf_align = 1; -return; -} +unsigned int sector_size = 0; Pointless initialization. If I do not initialize the sector_size, and the ioctl fails, I will return garbage as a blocksize to the caller. Where? As far as I can see, we return it only after ioctl() succeeded. /* Try a few ioctls to get the right size */ -bs-request_alignment = 0; -s-buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef CONFIG_XFS if (s-is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, da) = 0) { -bs-request_alignment = da.d_miniosz; +sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s-buf_align = da.d_mem; */ Since you keep the enabled assignments to s-buf_align out of this function, you should keep out this disabled one, too. +return sector_size; } } #endif +return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. +{ +unsigned int blk_size = 0; Pointless initialization. Same here. Again, we return it only after ioctl() succeeded. +#ifdef BLKPBSZGET +if (ioctl(fd, BLKPBSZGET, blk_size) = 0) { +return blk_size; +} +#endif + +return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ +BDRVRawState *s = bs-opaque; +char *buf; + +/* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ +if (bs-sg || !s-needs_alignment) { +bs-request_alignment = 1; +s-buf_align = 1; +return; +} + +s-buf_align = 0; +/* Let's try to use the logical blocksize for the alignment. */ +bs-request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s-buf_align) { size_t align;
Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
On 11/28/2014 03:52 PM, Markus Armbruster wrote: Ekaterina Tumanova tuman...@linux.vnet.ibm.com writes: Suggest function comment /** * Return logical block size, or zero if we can't figure it out */ { -BDRVRawState *s = bs-opaque; -char *buf; -unsigned int sector_size; - -/* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ -if (bs-sg || !s-needs_alignment) { -bs-request_alignment = 1; -s-buf_align = 1; -return; -} +unsigned int sector_size = 0; Pointless initialization. If I do not initialize the sector_size, and the ioctl fails, I will return garbage as a blocksize to the caller. Where? As far as I can see, we return it only after ioctl() succeeded. Sorry, you're absolutely right. I kept seeing and thinking that I always returned sector_size variable. ::facepalm:: /* Try a few ioctls to get the right size */ -bs-request_alignment = 0; -s-buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef CONFIG_XFS if (s-is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, da) = 0) { -bs-request_alignment = da.d_miniosz; +sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s-buf_align = da.d_mem; */ Since you keep the enabled assignments to s-buf_align out of this function, you should keep out this disabled one, too. +return sector_size; } } #endif +return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. +{ +unsigned int blk_size = 0; Pointless initialization. Same here. Again, we return it only after ioctl() succeeded. +#ifdef BLKPBSZGET +if (ioctl(fd, BLKPBSZGET, blk_size) = 0) { +return blk_size; +} +#endif + +return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ +BDRVRawState *s = bs-opaque; +char *buf; + +/* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ +if (bs-sg || !s-needs_alignment) { +bs-request_alignment = 1; +s-buf_align = 1; +return; +} + +s-buf_align = 0; +/* Let's try to use the logical blocksize for the alignment. */ +bs-request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s-buf_align) { size_t align;
Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
Ekaterina Tumanova tuman...@linux.vnet.ibm.com writes: Move the IOCTL calls that detect logical blocksize from raw_probe_alignment into separate function (probe_logical_blocksize). Introduce function which detect physical blocksize via IOCTL (probe_physical_blocksize). Both functions will be used in the next patch. The first one is used in this patch, actually. Signed-off-by: Ekaterina Tumanova tuman...@linux.vnet.ibm.com Subject's subsystem is geometry. Geometry is your topic. The subsystem is what you're patching. Here, it's raw-posix or block/raw-posix. Likewise for the other patches in this series. Stefan asked you move probe_physical_blocksize() to the patch that uses it, and I concur. With that done, I'd write a commit message like raw-posix: Factor block size detection out of raw_probe_alignment() Put it in new probe_logical_blocksize(). --- block/raw-posix.c | 58 +-- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index e100ae2..45f1d79 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename) } #endif -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. Suggest function comment /** * Return logical block size, or zero if we can't figure it out */ { -BDRVRawState *s = bs-opaque; -char *buf; -unsigned int sector_size; - -/* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ -if (bs-sg || !s-needs_alignment) { -bs-request_alignment = 1; -s-buf_align = 1; -return; -} +unsigned int sector_size = 0; Pointless initialization. /* Try a few ioctls to get the right size */ -bs-request_alignment = 0; -s-buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef CONFIG_XFS if (s-is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, da) = 0) { -bs-request_alignment = da.d_miniosz; +sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s-buf_align = da.d_mem; */ Since you keep the enabled assignments to s-buf_align out of this function, you should keep out this disabled one, too. +return sector_size; } } #endif +return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) Parameter bs is unused, let's drop it. +{ +unsigned int blk_size = 0; Pointless initialization. +#ifdef BLKPBSZGET +if (ioctl(fd, BLKPBSZGET, blk_size) = 0) { +return blk_size; +} +#endif + +return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ +BDRVRawState *s = bs-opaque; +char *buf; + +/* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ +if (bs-sg || !s-needs_alignment) { +bs-request_alignment = 1; +s-buf_align = 1; +return; +} + +s-buf_align = 0; +/* Let's try to use the logical blocksize for the alignment. */ +bs-request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s-buf_align) { size_t align;
Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
On Fri, Nov 21, 2014 at 11:17:02AM +0100, Christian Borntraeger wrote: Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: Move the IOCTL calls that detect logical blocksize from raw_probe_alignment into separate function (probe_logical_blocksize). Introduce function which detect physical blocksize via IOCTL (probe_physical_blocksize). Both functions will be used in the next patch. Signed-off-by: Ekaterina Tumanova tuman...@linux.vnet.ibm.com From what I can tell this should be a no-op for raw_probe_alignment. probe_physical_blocksize looks also good. When this patch is applied stand-alone, gcc will complain about a defined but unused function, though. So we might want to move this function into patch 3 or just add an __attribute__((unused)) here (and remove that in patch 3). Or just leave it as is. Please move probe_physical_blocksize() to Patch 3 because some QEMU builds default to -Werror where this patch causes a build failure. (In order for git-bisect(1) to work patches must not break the build.) Stefan pgpo4bEkT9sy2.pgp Description: PGP signature
Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions
Am 19.11.2014 um 11:17 schrieb Ekaterina Tumanova: Move the IOCTL calls that detect logical blocksize from raw_probe_alignment into separate function (probe_logical_blocksize). Introduce function which detect physical blocksize via IOCTL (probe_physical_blocksize). Both functions will be used in the next patch. Signed-off-by: Ekaterina Tumanova tuman...@linux.vnet.ibm.com From what I can tell this should be a no-op for raw_probe_alignment. probe_physical_blocksize looks also good. When this patch is applied stand-alone, gcc will complain about a defined but unused function, though. So we might want to move this function into patch 3 or just add an __attribute__((unused)) here (and remove that in patch 3). Or just leave it as is. Otherwise Reviewed-by: Christian Borntraeger borntrae...@de.ibm.com --- block/raw-posix.c | 58 +-- 1 file changed, 39 insertions(+), 19 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index e100ae2..45f1d79 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -223,50 +223,70 @@ static int raw_normalize_devicepath(const char **filename) } #endif -static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +static unsigned int probe_logical_blocksize(BlockDriverState *bs, int fd) { -BDRVRawState *s = bs-opaque; -char *buf; -unsigned int sector_size; - -/* For /dev/sg devices the alignment is not really used. - With buffered I/O, we don't have any restrictions. */ -if (bs-sg || !s-needs_alignment) { -bs-request_alignment = 1; -s-buf_align = 1; -return; -} +unsigned int sector_size = 0; /* Try a few ioctls to get the right size */ -bs-request_alignment = 0; -s-buf_align = 0; - #ifdef BLKSSZGET if (ioctl(fd, BLKSSZGET, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, sector_size) = 0) { -bs-request_alignment = sector_size; +return sector_size; } #endif #ifdef CONFIG_XFS if (s-is_xfs) { struct dioattr da; if (xfsctl(NULL, fd, XFS_IOC_DIOINFO, da) = 0) { -bs-request_alignment = da.d_miniosz; +sector_size = da.d_miniosz; /* The kernel returns wrong information for d_mem */ /* s-buf_align = da.d_mem; */ +return sector_size; } } #endif +return 0; +} + +static unsigned int probe_physical_blocksize(BlockDriverState *bs, int fd) +{ +unsigned int blk_size = 0; +#ifdef BLKPBSZGET +if (ioctl(fd, BLKPBSZGET, blk_size) = 0) { +return blk_size; +} +#endif + +return 0; +} + +static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) +{ +BDRVRawState *s = bs-opaque; +char *buf; + +/* For /dev/sg devices the alignment is not really used. + With buffered I/O, we don't have any restrictions. */ +if (bs-sg || !s-needs_alignment) { +bs-request_alignment = 1; +s-buf_align = 1; +return; +} + +s-buf_align = 0; +/* Let's try to use the logical blocksize for the alignment. */ +bs-request_alignment = probe_logical_blocksize(bs, fd); + /* If we could not get the sizes so far, we can only guess them */ if (!s-buf_align) { size_t align;