Re: [Qemu-devel] [PATCH v5] Support vhd type VHD_DIFFERENCING

[Stefan Hajnoczi <stefanha@gmail.com>]

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

On Wed, Oct 08, 2014 at 08:42:32PM +0800, Xiaodong Gong wrote:
> Now qemu only supports vhd type VHD_FIXED and VHD_DYNAMIC, so qemu
> can't read snapshot volume of vhd, and can't support other storage
> features of vhd file.
> 
> This patch add read parent information in function "vpc_open", read
> bitmap in "vpc_read", and change bitmap in "vpc_write".
> 
> Signed-off-by: Xiaodong Gong <gordongong0350@gmail.com>
> Reviewed-by: Ding xiao <ssdxiao@163.com>
> ---
>  block/vpc.c | 428 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 357 insertions(+), 71 deletions(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 4947369..1210542 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -29,17 +29,27 @@
>  #if defined(CONFIG_UUID)
>  #include <uuid/uuid.h>
>  #endif
> +#include <iconv.h>

This isn't used by the patch?

If you need to convert between character encodings use glib functions
instead of iconv:
https://developer.gnome.org/glib/2.26/glib-Character-Set-Conversion.html

>  
>  /**************************************************************/
>  
>  #define HEADER_SIZE 512
> +#define DYNAMIC_HEADER_SIZE 1024
> +#define PARENT_LOCATOR_NUM 8
> +#define MACX_PREFIX_LEN 7 /* file:// */
> +#define TBBATMAP_HEAD_SIZE 28
> +
> +#define PLATFORM_MACX 0x5863614d /* big endian */
> +#define PLATFORM_W2RU 0x75723257
> +
> +#define VHD_VERSION(major, minor)  (((major) << 16) | ((minor) & 0x0000FFFF))
>  
>  //#define CACHE
>  
>  enum vhd_type {
>      VHD_FIXED           = 2,
>      VHD_DYNAMIC         = 3,
> -    VHD_DIFFERENCING    = 4,
> +    VHD_DIFF            = 4,
>  };
>  
>  // Seconds since Jan 1, 2000 0:00:00 (UTC)
> @@ -138,6 +148,15 @@ typedef struct BDRVVPCState {
>      Error *migration_blocker;
>  } BDRVVPCState;
>  
> +typedef struct vhd_tdbatmap_header {
> +    char magic[8]; /* always "tdbatmap" */
> +
> +    uint64_t batmap_offset;
> +    uint32_t batmap_size;
> +    uint32_t batmap_version;
> +    uint32_t checksum;
> +} QEMU_PACKED VHDTdBatmapHeader;
> +
>  static uint32_t vpc_checksum(uint8_t* buf, size_t size)
>  {
>      uint32_t res = 0;
> @@ -153,10 +172,107 @@ static uint32_t vpc_checksum(uint8_t* buf, size_t size)
>  static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
>  {
>      if (buf_size >= 8 && !strncmp((char *)buf, "conectix", 8))
> -	return 100;
> +        return 100;
>      return 0;
>  }
>  
> +static int vpc_read_backing_loc(VHDDynDiskHeader *dyndisk_header,
> +                                BlockDriverState *bs,
> +                                Error **errp)
> +{
> +    BDRVVPCState *s = bs->opaque;
> +    int64_t data_offset = 0;
> +    int data_length = 0;
> +    uint32_t platform;
> +    bool done = false;
> +    int parent_locator_offset = 0;
> +    int i;
> +    int ret = 0;
> +
> +    for (i = 0; i < PARENT_LOCATOR_NUM; i++) {
> +        data_offset =
> +            be64_to_cpu(dyndisk_header->parent_locator[i].data_offset);
> +        data_length =
> +            be32_to_cpu(dyndisk_header->parent_locator[i].data_length);
> +        platform = dyndisk_header->parent_locator[i].platform;

be32_to_cpu() missing?

> +
> +        /* Extend the location offset */
> +        if (parent_locator_offset < data_offset) {
> +            parent_locator_offset = data_offset;
> +        }
> +
> +        if (done) {
> +            continue;
> +        }
> +
> +        /* Skip "file://" in MacX platform */
> +        if (platform == PLATFORM_MACX) {
> +            data_offset += MACX_PREFIX_LEN;
> +            data_length -= MACX_PREFIX_LEN;
> +        }

Please check data_length >= MACX_PREFIX_LEN.  It's error-prone to allow
data_length to underflow.

> +
> +        /* Read location of backing file */
> +        if (platform == PLATFORM_MACX || platform == PLATFORM_W2RU) {
> +            if (data_offset > s->max_table_entries * s->block_size) {
> +                return -1;
> +            }
> +            if (data_length > BDRV_SECTOR_SIZE) {
> +                return -1;
> +            }
> +            ret = bdrv_pread(bs->file, data_offset, bs->backing_file,
> +                data_length);

Please check data_length against bs->backing_file[] size before reading
into it.

> +            if (ret < 0) {
> +                return ret;
> +            }
> +            bs->backing_file[data_length] = '\0';
> +        }
> +
> +        /* Convert location to ACSII string */
> +        if (platform == PLATFORM_MACX) {
> +            done = true;
> +
> +        } else if (platform == PLATFORM_W2RU) {
> +            /* Must be UTF16-LE to ASCII */

I guess this is where you wanted to use iconv?

> +            char *out, *optr;
> +            int j;
> +
> +            optr = out = (char *) malloc(data_length + 1);

Please use g_malloc()/g_free() instead of malloc()/free().

> +            if (out == NULL) {
> +                ret = -1;
> +                return ret;
> +            }
> +            memset(out, 0, data_length + 1);
> +
> +            for (j = 0; j < data_length + 1; j++) {
> +                out[j] = bs->backing_file[2*j];
> +            }
> +            out[data_length + 1] = '\0';
> +
> +            while (*optr != '\0') {
> +                if (*optr == '\\') {
> +                    *optr = '/';
> +                }
> +                optr++;
> +            }
> +
> +            strncpy(bs->backing_file, out, data_length + 1);
> +
> +            out = NULL;
> +            free(out);

Did you mean:
free(out);
out = NULL;
?

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