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

[Dave Allan]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list