Re: [libvirt] [PATCHv2 1/2] virDomainSaveXML: Reject domains which name contain '/'

On 02/07/2013 06:27 AM, Christophe Fergeau wrote: 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