|
From: | Ekaterina Tumanova |
Subject: | Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions |
Date: | Fri, 28 Nov 2014 16:28:52 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
On 11/28/2014 03:52 PM, Markus Armbruster wrote:
Ekaterina Tumanova <address@hidden> 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, §or_size) >= 0) { - bs->request_alignment = sector_size; + return sector_size; } #endif #ifdef DKIOCGETBLOCKSIZE if (ioctl(fd, DKIOCGETBLOCKSIZE, §or_size) >= 0) { - bs->request_alignment = sector_size; + return sector_size; } #endif #ifdef DIOCGSECTORSIZE if (ioctl(fd, DIOCGSECTORSIZE, §or_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;
[Prev in Thread] | Current Thread | [Next in Thread] |