From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJFq-00073K-Ux for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:58:35 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XuJFh-0006O1-St for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:58:26 -0500 Received: from e06smtp17.uk.ibm.com ([195.75.94.113]:48628) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XuJFh-0006Np-Ii for qemu-devel@nongnu.org; Fri, 28 Nov 2014 05:58:17 -0500 Received: from /spool/local by e06smtp17.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Nov 2014 10:58:16 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 852472190046 for ; Fri, 28 Nov 2014 10:57:46 +0000 (GMT) Received: from d06av12.portsmouth.uk.ibm.com (d06av12.portsmouth.uk.ibm.com [9.149.37.247]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sASAwEoX19267698 for ; Fri, 28 Nov 2014 10:58:14 GMT Received: from d06av12.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av12.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sASAwD6R004220 for ; Fri, 28 Nov 2014 03:58:13 -0700 Message-ID: <54785544.5020904@linux.vnet.ibm.com> Date: Fri, 28 Nov 2014 13:58:12 +0300 From: Ekaterina Tumanova MIME-Version: 1.0 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> In-Reply-To: <874mtkczhu.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Markus Armbruster Cc: kwolf@redhat.com, dahi@linux.vnet.ibm.com, Public KVM Mailing List , mihajlov@linux.vnet.ibm.com, borntraeger@de.ibm.com, stefanha@redhat.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com > > 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, §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. > >> +#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; >