Re: [Qemu-devel] [PATCH v5 3/5] block: Add driver methods to probe blocksizes and geometry

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2161 bytes --]

On Thu, Dec 18, 2014 at 12:18:02PM +0100, Ekaterina Tumanova wrote:
> @@ -90,6 +91,10 @@
>  #include <xfs/xfs.h>
>  #endif
>  
> +#ifdef __s390__
> +#include <asm/dasd.h>
> +#endif

#if defined(__linux__) && defined(__s390__)

Or you could move it inside #ifdef __linux__ earlier in the file.

> +/**
> + * Try to get @bs's geometry (cyls, heads, sectos)
> + * On success, store them in @geo and return 0.
> + * On failure return -errno.
> + * (as for 12/2014 only DASDs are supported)

This comment isn't useful because it's apparent from the code and it
could get outdated.  It might be better to remove it and instead
document that this function allows the block driver to assign default
geometry values that the guest sees.

> + */
> +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 -ENOTSUP;
> +    }
> +    if (ioctl(s->fd, HDIO_GETGEO, &ioctl_geo) < 0) {

This is a Linux ioctl, won't this break compilation on non-Linux hosts?

> +        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 -ENOTSUP;
> +    }
> +    /* Do not return a geometry for partition */
> +    if (ioctl_geo.start != 0) {
> +        return -ENOTSUP;
> +    }
> +    geo->heads = ioctl_geo.heads;
> +    geo->sectors = ioctl_geo.sectors;
> +    if (!probe_physical_blocksize(s->fd, &blksize)) {
> +        /* overwrite cyls: HDIO_GETGEO result is incorrect for big drives */
> +        geo->cylinders = bs->total_sectors / (blksize / BDRV_SECTOR_SIZE)
> +                                           / (geo->heads * geo->sectors);

Please use bdrv_nb_sectors(bs) instead of bs->total_sectors.
bs->total_sectors is a cached value that is basically private to
block.c and should not be accessed directly.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]