On 02/07/2013 06:27 AM, Christophe Fergeau wrote: > Similarly to 790f912b4 which rejects snapshots names containing, > this commit changes virDomainSaveXML to reject domains with a '/' > in their name. The domain name is used as a filename, so this > leads to unexpected results when used in combination with '..' > --- > src/conf/domain_conf.c | 7 +++++++ > 1 file changed, 7 insertions(+) I like the end goal, but am not sure the approach was right. That is, adding the checks to domain_conf.c feels too strong. I can envision the set of allowed vs. restricted names being different for different hypervisors - just because the qemu driver creates a .log file based on the domain name doesn't mean that other drivers will do the same (I don't have enough experience with ESX to say for certain, but as far as I know, libvirt doesn't create any additional files based on domain names for esx:// connections and therefore does not need to restrict '/'; or ESX may have its own set of even tighter rules on what forms a valid name). I think the approach in commit 790f912b4 of sticking the restriction in qemu_driver.c is better; the parser should accept everything, and then the driver should decide whether it was valid. Also, I see no reason to split this into two patches; adding the restrictions in the snapshot case was split into two patches only because of a late-breaking review. And I'm not sure that VIR_ERR_XML_DETAIL was the best choice of error message in the earlier patches that you copied from, although fixing it to something nicer would be a separate cleanup. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list