Re: [Qemu-devel] [PATCH] Add basic read, write and create support for AMD SimNow HDD images.

[Stefan Hajnoczi]

On Sun, Nov 28, 2010 at 7:08 PM, François Revol <address@hidden> wrote:
> From b0602bc2b02dcd7b15f0f9a143f850defd767509 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Fran=C3=A7ois=20Revol?= <address@hidden>
> Date: Sun, 28 Nov 2010 20:01:03 +0100
> Subject: [PATCH] Add basic read, write and create support for AMD SimNow HDD 
> images.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
>
> Signed-off-by: François Revol <address@hidden>
> ---
>  Makefile.objs |    2 +-
>  block/hdd.c   |  354 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 355 insertions(+), 1 deletions(-)
>  create mode 100644 block/hdd.c

This block driver does not implement the asynchronous APIs
(bdrv_aio_writev, bdrv_aio_readv, bdrv_aio_flush) which are necessary
for running a VM properly.  Some block drivers are currently written
without async support and that limits them to being used as qemu-img
formats.  It's a bad idea to run a VM with these block drivers because
I/O will block the VM from making progress (it is synchronous).

Anthony mentioned this on IRC and you explained that this requirement
isn't obvious from the QEMU source.  Luckily the changes needed are
small so it's definitely worth making SimNow HDD images async.

> +typedef struct BDRVHddState {
> +    uint8_t identify_data[SECTOR_SIZE];

Perhaps identify_data[] should be uint16_t since it gets casted on every use.

> +static void padstr(char *str, const char *src, int len)
> +{
> +    int i, v;
> +    for(i = 0; i < len; i++) {
> +        if (*src)
> +            v = *src++;
> +        else
> +            v = ' ';
> +        str[i^1] = v;
> +    }
> +}

This function is confusing, it uses int v instead of char.  The name
padstr() doesn't hint that it also byteswaps the string.

QEMU coding style uses {} even for one-line if statement bodies
(Section 4 in the CODING_STYLE file).

> +static int hdd_probe(const uint8_t *buf, int buf_size, const char *filename)
> +{
> +    int name_len;
> +    uint16_t *p = (uint16_t *)buf;
> +    int64_t nb_sectors;
> +    uint32_t nb_sectors_clipped;
> +    int result = 0;
> +    int i;
> +
> +    if (buf_size < SECTOR_SIZE) {
> +        /* Header too small, no VDI. */
> +        return 0;
> +    }
> +
> +    /* best effort sanity check */
> +    /* TODO: check more (CHS size...) */
> +
> +    /* serial number */
> +    for (i = 10 * 2; i < 10 * 2 + 20; i++) {
> +        if (!isvalid_ide_chr(buf[i])) {
> +            return 0;
> +        }
> +    }
> +    result += 20;
> +
> +    /* firmware version */
> +    for (i = 23 * 2; i < 23 * 2 + 8; i++) {
> +        if (!isvalid_ide_chr(buf[i])) {
> +            return 0;
> +        }
> +    }
> +    result += 8;
> +
> +    /* model */
> +    for (i = 27 * 2; i < 27 * 2 + 40; i++) {
> +        if (!isvalid_ide_chr(buf[i])) {
> +            return 0;
> +        }
> +    }
> +    result += 40;
> +
> +    nb_sectors = le16_to_cpu(p[100]);
> +    nb_sectors |= (uint64_t)le16_to_cpu(p[101]) << 16;
> +    nb_sectors |= (uint64_t)le16_to_cpu(p[102]) << 32;
> +    nb_sectors |= (uint64_t)le16_to_cpu(p[103]) << 48;
> +
> +    nb_sectors_clipped = le16_to_cpu(p[60]) | (le16_to_cpu(p[61]) << 16);
> +
> +    if (nb_sectors < 1 || ((uint32_t)nb_sectors) != nb_sectors_clipped) {
> +        return 0;
> +    }
> +    result += 10;
> +
> +    if (filename != NULL) {
> +        name_len = strlen(filename);
> +        if (name_len > 4 && !strcmp(filename + name_len - 4, ".hdd"))
> +            result += 20;
> +    }
> +
> +    return result;
> +}

HDD has no magic by which to identify valid files.  We need to avoid
false positives because existing image files could be corrupted or VMs
at least made unbootable.  Although using filename extensions to test
for formats is lame, in this case I don't think we have another
choice.

The result variable calculations are not needed.  Either we reject the
file and return 0, or we end up having added the same constants
(result = 20 + 8 + 40 + 10 + (endswith(filename, ".hdd") ? 20 : 0)).

> +    int sectors;
> +    int cylinders;
> +    int heads;

Unused variables?

> +    if (bdrv_read(bs->file, 0, s->identify_data, 1) < 0) {
> +        goto fail;
> +    }

We're assuming that BDRV_SECTOR_SIZE == SECTOR_SIZE == 512 throughout
the code.  It would be safer to explicitly calculate against
BDRV_SECTOR_SIZE.  It would be clearer to rename SECTOR_SIZE to
ATA_IDENTIFY_SIZE.

> +    if (hdd_probe(s->identify_data, SECTOR_SIZE, NULL) == 0) {
> +        goto fail;
> +    }
[...]
> + fail:
> +    return -1;

Please don't throw away specific error values.

> +    /* hints */
> +    /*
> +    bs->cyls = le16_to_cpu(p[54]);
> +    bs->heads = le16_to_cpu(p[55]);
> +    bs->secs = le16_to_cpu(p[56]);
> +    */

Deadcode.

> +static int hdd_read(BlockDriverState *bs, int64_t sector_num,
> +                    uint8_t *buf, int nb_sectors)
> +{
> +    int ret;
> +    if (bdrv_read(bs->file, sector_num + DATA_START, buf, nb_sectors) < 0) {
> +        return -1;
> +    }
> +    return 0;
> +}

Replace with async version:

static int hdd_aio_readv(BlockDriverState *bs, int64_t sector_num,
QEMUIOVector *qiov, int nb_sectors, BlockDriverCompletionFunc *cb,
void *opaque)
{
    return bdrv_aio_readv(bs->file, sector_num + DATA_START, qiov,
nb_sectors, cb, opaque);
}

> +static int hdd_write(BlockDriverState *bs, int64_t sector_num,
> +                    const uint8_t *buf, int nb_sectors)
> +{
> +    int ret;
> +
> +    if (sector_num > bs->total_sectors) {
> +        fprintf(stderr,
> +                "(HDD) Wrong offset: sector_num=0x%" PRIx64
> +                " total_sectors=0x%" PRIx64 "\n",
> +                sector_num, bs->total_sectors);
> +        return -1;
> +    }

Not needed since block.c already checks request range.

> +    /* TODO: check if already allocated, else truncate() */
> +    if (bdrv_write(bs->file, sector_num + DATA_START, buf, nb_sectors) < 0) {
> +        return -1;
> +    }
> +    return 0;
> +}

Should also be replaced with async version.

bdrv_aio_flush() is trivial too.  It is necessary for running VMs
since they may rely on flush working for data integrity when a mode
other than -drive ...,cache=writethrough is selected.

> +    fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC | O_BINARY | 
> O_LARGEFILE,
> +              0644);
> +    if (fd < 0) {
> +        return -errno;
> +    }

Please use bdrv_create_file(), bdrv_file_open(), and bdrv_pwrite()
instead of POSIX file I/O.  See block/qcow2.c:qcow_create2 as an
example.

> +    /* TODO: specs says it can grow, so no need to always do this */
> +    if (static_image) {
> +        if (ftruncate(fd, sizeof(header) + blocks * SECTOR_SIZE)) {
> +            result = -errno;
> +        }
> +    }

Is there an issue with leaving ftruncate() out?  I don't see a reason
to truncate.  If we want to preallocate then ftruncate() isn't
appropriate anyway.

> +static void hdd_close(BlockDriverState *bs)
> +{
> +    BDRVHddState *s = bs->opaque;

Unused variable.

Stefan