Re: [libvirt] [PATCHv2] xml: print uuids or shell-escaped names in the warning

On 10/29/12 10:38, Jiri Denemark wrote: >On Thu, Oct 25, 2012 at 16:37:37 +0200, Ján Tomko wrote: >>In the XML warning, this prints uuids for domain names with special >>characters in them and shell-escaped names for other elements (like >>snapshosts and networks) with special names. >>--- >>When saving snapshots, the domain name is appended to the >>"snapshot-edit" command, so using a domain name that needs escaping >>would lead to something that can't be just fed to the shell as it would >>glue them together. >> >>diff to xml: shell-escape domain name in comment: >>- Domain names don't get escaped, UUIDs are preferred. >>- The command gets escaped too. >> >>diff to v1: don't try to use CDATA (it doesn't belong there) >>--- >> src/conf/domain_conf.c | 6 +++- >> src/libvirt_private.syms | 2 + >> src/qemu/qemu_domain.c | 3 +- >> src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ >> src/util/buf.h | 1 + >> src/util/xml.c | 62 ++++++++++++++++++++++--------------------- >> src/util/xml.h | 1 + >> 7 files changed, 109 insertions(+), 32 deletions(-) > >I think we're making this too complicated for no real benefit. The goal is to >provide a hint to anyone who looks at the autogenerated XML files and IMHO >providing an escaped string (that would only work in environments for which it >was designed). I would just keep it simple: > >- emit "virsh command name" in case name is nice >- emit "virsh command uuid" in case name is ugly and uuid is known >- emit "virsh command" in all other cases This looks reasonable. > >This should keep the hints in domain and network XML files in /etc/libvirt >usable for copy&paste (the UUID fallback works there). Snapshot files (located >in /var/lib/libvirt) use "virsh snapshot-edit domain snapshot", where domains >are passed as part of the command and snapshots have no UUIDs. Thus to keep the >code simple, I'd emit just "virsh snapshot-edit", which is still a useful >hint and I don't believe we need to do anything beyond that. Well, we might check if the names contain dashes, and then just print the line without the names. But that's probably not worth even trying. Putting the static hint should be good enough. I'd just put there tokens that the user should replace for real values: "virsh snapshot-edit 'domain-name' 'snapshot-name'" or something like that.

A 109+/32- patch doesn't look worth of the usability improvement that is marginal. Peter -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list