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;
