From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57939) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0WlB-0007wv-RD for qemu-devel@nongnu.org; Mon, 15 Dec 2014 09:36:38 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0Wl2-00062Y-NZ for qemu-devel@nongnu.org; Mon, 15 Dec 2014 09:36:29 -0500 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:50100) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0Wl2-00062F-Bt for qemu-devel@nongnu.org; Mon, 15 Dec 2014 09:36:20 -0500 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 15 Dec 2014 14:36:17 -0000 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id F00DD1B08051 for ; Mon, 15 Dec 2014 14:36:39 +0000 (GMT) Received: from d06av04.portsmouth.uk.ibm.com (d06av04.portsmouth.uk.ibm.com [9.149.37.216]) by b06cxnps4075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id sBFEaG2J56754352 for ; Mon, 15 Dec 2014 14:36:16 GMT Received: from d06av04.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av04.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id sBFEaFar003876 for ; Mon, 15 Dec 2014 07:36:16 -0700 Message-ID: <548EF1DC.2000803@linux.vnet.ibm.com> Date: Mon, 15 Dec 2014 17:36:12 +0300 From: Ekaterina Tumanova MIME-Version: 1.0 References: <1417802181-20834-1-git-send-email-tumanova@linux.vnet.ibm.com> <1417802181-20834-4-git-send-email-tumanova@linux.vnet.ibm.com> <87tx0xhwib.fsf@blackfin.pond.sub.org> In-Reply-To: <87tx0xhwib.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/5] block: Add driver methods to probe blocksizes and geometry 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 On 12/15/2014 04:29 PM, Markus Armbruster wrote: > Ekaterina Tumanova writes: > >> This patch introduces driver methods of defining disk blocksizes >> (physical and logical) and hard drive geometry. >> The method is only implemented for "host_device". For "raw" devices >> driver calls child's method. >> >> For the time being geometry detection will only work for DASD devices. >> In order to check that a local check_for_dasd function was introduced, >> which calls BIODASDINFO2 ioctl and returns its rc. >> >> Blocksizes detection fuction will probe sizes for DASD devices and >> set default for other devices. >> >> Signed-off-by: Ekaterina Tumanova >> --- >> block/raw-posix.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> block/raw_bsd.c | 14 +++++++++ >> 2 files changed, 105 insertions(+) >> >> diff --git a/block/raw-posix.c b/block/raw-posix.c >> index 633d5bc..33f9983 100644 >> --- a/block/raw-posix.c >> +++ b/block/raw-posix.c >> @@ -56,6 +56,7 @@ >> #include >> #include >> #include >> +#include >> #ifndef FS_NOCOW_FL >> #define FS_NOCOW_FL 0x00800000 /* Do not cow file */ >> #endif >> @@ -90,6 +91,10 @@ >> #include >> #endif >> >> +#ifdef __s390__ >> +#include >> +#endif >> + >> //#define DEBUG_FLOPPY >> >> //#define DEBUG_BLOCK >> @@ -242,6 +247,20 @@ static int probe_logical_blocksize(int fd, unsigned int *sector_size) >> return 0; >> } >> >> +/* >> + * Set physical block size via ioctl. On success return 0. Otherwise -errno. >> + */ > > Set? > > Ah, now I get your logic, you mean "set *blk_size"! > > Suggest to say it like this: > > /** > * Get physical block size of @fd. > * On success, store it in @blk_size and return 0. > * On failure, return -errno. > */ > >> +static int probe_physical_blocksize(int fd, unsigned int *blk_size) >> +{ >> +#ifdef BLKPBSZGET >> + if (ioctl(fd, BLKPBSZGET, blk_size) < 0) { >> + return -errno; >> + } >> +#endif >> + >> + return 0; > > If !defined(BLKPBSZGET), you return 0 without setting *blk_size. I > think you need to fail then. -ENOTSUP should do. > >> +} >> + >> static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp) >> { >> BDRVRawState *s = bs->opaque; >> @@ -662,6 +681,76 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp) >> bs->bl.opt_mem_alignment = s->buf_align; >> } >> >> +static int check_for_dasd(int fd) >> +{ >> +#ifdef BIODASDINFO2 >> + struct dasd_information2_t info = {0}; >> + >> + return ioctl(fd, BIODASDINFO2, &info); >> +#endif >> + return -1; >> +} >> + >> +/* >> + * Try to get the device blocksize. On success 0. On failure return -errno. >> + * Currently only implemented for DASD drives. >> + */ > > Compare to the function contract I wrote for the callback in my review > of PATCH 1. If you like that one better, feel free to reuse it here. > >> +static int hdev_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + BDRVRawState *s = bs->opaque; >> + int ret; >> + >> + /* If DASD, get blocksizes */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; > > This is not a negative error code! -ENOTSUP. > >> + } >> + ret = probe_logical_blocksize(s->fd, &bsz->log); >> + if (ret < 0) { >> + return ret; >> + } >> + return probe_physical_blocksize(s->fd, &bsz->phys); >> +} >> + >> +/* >> + * Try to get the device geometry. On success 0. On failure return -errno. >> + * Currently only implemented for DASD drives. >> + */ > > Compare to the function contract I wrote for the callback in my review > of PATCH 1. If you like that one better, feel free to reuse it here. > >> +static int hdev_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + BDRVRawState *s = bs->opaque; >> + struct hd_geometry ioctl_geo = {0}; >> + uint32_t blksize; >> + >> + /* If DASD, get it's geometry */ >> + if (check_for_dasd(s->fd) < 0) { >> + return -1; > > This is not a negative error code! -ENOTSUP. > >> + } >> + if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) { >> + return -errno; >> + } >> + /* HDIO_GETGEO may return success even though geo contains zeros >> + (e.g. certain multipath setups) */ >> + if (!ioctl_geo.heads || !ioctl_geo.sectors || !ioctl_geo.cylinders) { >> + return -1; > > Need an error code. > >> + } >> + /* Do not return a geometry for partition */ >> + if (ioctl_geo.start != 0) { >> + return -1; > > Need an error code. > >> + } >> + geo->heads = ioctl_geo.heads; >> + geo->sectors = ioctl_geo.sectors; >> + if (bs->total_sectors) { >> + if (!probe_physical_blocksize(s->fd, &blksize)) { >> + geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE) >> + / (geo->heads * geo->sectors); >> + return 0; >> + } >> + } > > How could !bs->total_sectors happen? > >> + geo->cylinders = ioctl_geo.cylinders; >> + >> + return 0; >> +} >> + >> static ssize_t handle_aiocb_ioctl(RawPosixAIOData *aiocb) >> { >> int ret; >> @@ -2127,6 +2216,8 @@ static BlockDriver bdrv_host_device = { >> .bdrv_get_info = raw_get_info, >> .bdrv_get_allocated_file_size >> = raw_get_allocated_file_size, >> + .bdrv_probe_blocksizes = hdev_probe_blocksizes, >> + .bdrv_probe_geometry = hdev_probe_geometry, >> >> .bdrv_detach_aio_context = raw_detach_aio_context, >> .bdrv_attach_aio_context = raw_attach_aio_context, >> diff --git a/block/raw_bsd.c b/block/raw_bsd.c >> index 401b967..cfd5249 100644 >> --- a/block/raw_bsd.c >> +++ b/block/raw_bsd.c >> @@ -173,6 +173,18 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename) >> return 1; >> } >> >> +static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz) >> +{ >> + bdrv_probe_blocksizes(bs->file, bsz); >> + >> + return 0; >> +} > > Correct, just a bit awkward due to the difference between > bdrv_probe_blocksizes() and ->bdrv_probe_blocksizes(). I guess I'd make > them the same, but it's your patch. > I think would stick with my approach. You mentioned during review of v2, that blk_probe_blocksizes should always just work. And I think it's right. An error code from a method allows me to set a default in the wrapper. So wrapper will never fail. Thanks a lot for your review! I'll try to post a new version ASAP. >> + >> +static int raw_probe_geometry(BlockDriverState *bs, hdGeometry *geo) >> +{ >> + return bdrv_probe_geometry(bs->file, geo); >> +} >> + >> static BlockDriver bdrv_raw = { >> .format_name = "raw", >> .bdrv_probe = &raw_probe, >> @@ -190,6 +202,8 @@ static BlockDriver bdrv_raw = { >> .has_variable_length = true, >> .bdrv_get_info = &raw_get_info, >> .bdrv_refresh_limits = &raw_refresh_limits, >> + .bdrv_probe_blocksizes = &raw_probe_blocksizes, >> + .bdrv_probe_geometry = &raw_probe_geometry, >> .bdrv_is_inserted = &raw_is_inserted, >> .bdrv_media_changed = &raw_media_changed, >> .bdrv_eject = &raw_eject, >