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

[Xiaodong Gong]

On 10/28/14, Stefan Hajnoczi <address@hidden> wrote:
> 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 <address@hidden>
>> Reviewed-by: Ding xiao <address@hidden>
>> ---
>>  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?

this platform is big-ending

>
>> +
>> +        /* 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.

upper data_length > BDRV_SECTOR_SIZE get this done

>
>> +            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?

I used the iconv first time, but changed it to the following things.
There are tow reasons, it could fail because the right codeset packet
is not installed and it must be UTF16-LE to ASCII. How about your ?

>
>> +            char *out, *optr;
>> +            int j;
>> +
>> +            optr = out = (char *) malloc(data_length + 1);
>
> Please use g_malloc()/g_free() instead of malloc()/free().
>

I will change it to g_malloc/g_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;
> ?

it is directly a bug.

>