From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42243) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuL2t-0000r1-Ks for qemu-devel@nongnu.org; Fri, 28 Nov 2014 07:53:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XuL2n-0001Nw-DO for qemu-devel@nongnu.org; Fri, 28 Nov 2014 07:53:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60108) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuL2n-0001Ns-6K for qemu-devel@nongnu.org; Fri, 28 Nov 2014 07:53:05 -0500 From: Markus Armbruster References: <1416392276-10408-1-git-send-email-tumanova@linux.vnet.ibm.com> <1416392276-10408-3-git-send-email-tumanova@linux.vnet.ibm.com> <874mtkczhu.fsf@blackfin.pond.sub.org> <54785544.5020904@linux.vnet.ibm.com> Date: Fri, 28 Nov 2014 13:52:56 +0100 In-Reply-To: <54785544.5020904@linux.vnet.ibm.com> (Ekaterina Tumanova's message of "Fri, 28 Nov 2014 13:58:12 +0300") Message-ID: <87sih35vx3.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v2 2/6] geometry: Detect blocksize via ioctls in separate static functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ekaterina Tumanova Cc: kwolf@redhat.com, borntraeger@de.ibm.com, Public KVM Mailing List , mihajlov@linux.vnet.ibm.com, dahi@linux.vnet.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com Ekaterina Tumanova 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, §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; >>