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

[Philipp Hahn]

Hello,

I'm no qemu-devel expert, but as I tried myself at adding VHD_DIFF
support some time ago, here are some comments:

On 08.09.2014 16:41, Xiaodong Gong wrote:
> diff --git a/block/vpc.c b/block/vpc.c
...
> +        /* Read backing file location from dyn header table */
> +        if (dyndisk_header->parent_name[0] || 
> dyndisk_header->parent_name[1]) {
> +            for (i = 0; i < PARENT_LOCATOR_NUM; i++) {
...
> +                if (MACX == platform) {

Most images I have luckily have the MACX entry, but from reading the
spec I think this is not guaranteed. What about the other more
Windows-agnostic types?

There's also dyndisk_header->parent_name (which uses UTF-16-BE).

> +                    ret = bdrv_pread(bs->file, data_offset + 
> PARENT_PREFIX_LEN,
> +                        bs->backing_file, data_length - PARENT_PREFIX_LEN);

You assume that the system locale is UTF-8 (which MACX uses as the
encoding). I don't know how QEMU handles that internally, but AFAIK for
correctness you would need to convert that from UTF-8 to $LC_CTYPE.
Using iconv() is currently is not possible, since it requires calling
setlocale() to be called from all main programs using that code.

>  static int vpc_write(BlockDriverState *bs, int64_t sector_num,
...
> +    bool isdiff = true;
...
> +            if (true == isdiff) {

 if (isdiff) {
is enough - no need to add any confusing ==true IMHO.


Other notes:
- Using backing files requires CONFIG_UUID. I once created a VHD file
using qemu-img, which set UUID:=0. This lead to Xen handling the image
as a raw-file instead of a vhd file instead.

Otherwise thank you for working on VHD support.

Sincerely
Philipp