[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



On 10/29/2012 10:38 AM, 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 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.
> 

Since the comment is merely a warning for people that aren't used to the
commands or don't know how libvirt works, I second that opinion.  I
myself believe there is no need for the whole command anyway, especially
when getting to know how to specify the right command-line encourages
the user to get to know virsh better.

Martin


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]