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
