On Mon, Oct 29, 2012 at 11:07:20AM +0100, Peter Krempa wrote: > 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. I agree; I'm pretty confident that people will understand what's meant by that and the additional complexity of adding the actual object name seems excessive. Dave > A 109+/32- patch doesn't look worth of the usability improvement > that is marginal. > > Peter > > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list