Re: [libvirt] [PATCH 14/20] snapshot: qemu: Add support for external inactive snapshots

On 10/23/2012 09:12 AM, Peter Krempa wrote: This doesn't match the actual code, which also adds '-f format' arguments. Also, see below about an incomplete argument. This seems awkward. I guess I see why you're preparing the list up front (because we are creating different commands in a loop, and because of copy-and-paste from earlier code), but I wonder if it would be simpler to create a virCommand from scratch on each iteration of the loop instead of reusing qemuimgarg. Hmm, I wonder if that's the right thing to do? If the file already exists, then it seems like we should just skip the qemu-img call altogether, and use the file as-is (trusting that the user created it correctly, the same as we would do for an active disk snapshot), rather than unlink()ing and then recreating the file. Also, unlink() is unsafe if the existing file is a block device instead of a regular file. This is a reintroduction of a backing file probing CVE if we know the file format of defdisk->src. You need to pass -o backing_file=%s,backing_fmt=%s with the backing format, if it is known, otherwise, we have to probe a file type where we previously knew the file type. I didn't look closely at the rest of the patch, but want to make sure we avoid a security hole. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org