Re: [libvirt] [PATCH v3 1/2] helper of copy-storage-* features

On 11/08/2012 10:26 PM, li guang wrote: >>> +{ >>> + char *tmp_dir = NULL, *outbuf = NULL; >>> + char *img_tool = driver->qemuImgBinary; >> >> Don't grab this field directly. Instead, use qemuFindQemuImgBinary(driver). > driver->qemuImgBinary is useful, I've initiated it. > why don't we use it? In the order your series was written, driver->qemuImgBinary may be uninitialized. If you swap the order and put patch 2/2 first, then yes, you could use this field directly instead of calling qemuFindQemuImgBinary. You should always try to order your patches so that no patch is broken until later patches in the series have been applied. >>> + if (gen_del) { >>> + cmd = virCommandNewArgList(img_tool, "create", "-f", >>> + def->disks[i]->driverType, def->disks[i]->src, NULL); >>> + if (STREQ(def->disks[i]->driverType, "qcow2") || >>> + STREQ(def->disks[i]->driverType, "qcow")) >>> + virCommandAddArgFormat(cmd, "%lluK", 0xffffffffff); >> >> Ouch. You should not be passing in random sizes to qemu - if you need >> to know the source size in order to create a file on the destination >> side, then we HAVE to modify the migration cookie in order to pass >> sizing information when the new flag is present. >> > passing by cookie seems a little rough, > these sizes are not random, they're > the max size of their format. > does setting max size hurt? Yes, because it is wrong. > the disk images are sparse files, So?

A user that calls virsh vol-info on the file on the destination side should not get infinite size, but the same size as they would get on the source. Just because it doesn't (yet) occupy the entire disk doesn't mean that we should size it so large that the file can grow without effective bounds and cause ENOSPC.

> and after qemu migration finished, size will be right. If qemu changes the size after migration, then why can't it change from 0 to the proper size, rather than from max to the proper size?