Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions

[Ekaterina Tumanova]

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;