[libvirt] [PATCH v2 08/10] virt-admin: Wire-up the logging APIs

[John Ferlan]

On 12/13/2016 09:32 AM, Erik Skultety wrote:
> On Mon, Dec 12, 2016 at 11:21:08AM -0500, John Ferlan wrote:
>>
>>
>> On 12/12/2016 10:42 AM, Erik Skultety wrote:
>>> On Fri, Dec 09, 2016 at 07:29:36AM -0500, John Ferlan wrote:
>>>>
>>>>
>>>> On 11/25/2016 08:12 AM, Erik Skultety wrote:
>>>>> Finally, now that all APIs have been introduced, wire them up to virt-admin
>>>>> and introduce daemon-log-outputs and daemon-log-filters commands.
>>>>>
>>>>> Signed-off-by: Erik Skultety <eskultet at redhat.com>
>>>>> ---
>>>>>  tools/virt-admin.c   | 120 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  tools/virt-admin.pod |  90 ++++++++++++++++++++++++++++++++++++++
>>>>>  2 files changed, 210 insertions(+)
>>>>>
>>>>
>>>> Now it *is* the coffee - should there be any concerns along the way over
>>>> escaping characters?  I think this is the only place where we
>>>> read/print, but via the API if someone passes a string - do we need to
>>>> worry about that? Yes, I know - not the normal thing, but you know
>>>> someone will try...
>>>
>>> Is it an issue though? If someone sends a binary garbage for filename, we have
>>> to create a logfile with that name on the filesystem. When we retrieve it, I
>>> think we should print it as we created it - garbage. The other option we have
>>> is to strip all the control characters and print the rest which in the worst
>>> case scenario will end up as an empty string...
>>>
>>
>> If someone provides garbage I thought our general practice was to escape
>> the 'unprintable' chars when we Format the output.
> 
> In which case? Yes, we do it for XMLs (hence the function virBufferEscapeString)
> so that the resulting XML is compliant with the specification. We also do it
> for shell-related stuff like escaping the arguments that will end up on qemu
> cmdline. But what's the purpose of escaping in this case? Additionally, if you
> escape the output and let's say the user saved the output into a environment
> variable, he now has an escaped garbage which is further unusable when instead
> we should have gone with the verbatim, unprintable garbage.
> I played around with this in the morning and found out that bash allows you to
> use structure like this one: $'nnn' (where nnn is the string to be escaped),
> now that would be doable from our perspective, but then, I'm not sure whether
> such a construction is supported with other shells out there.
> 

Escaping wasn't a requirement, just a concern. Hence the original
question "should there be any concerns along the way over
escaping characters? "

And by escaping I meant only the name of the file, although perhaps that
wasn't clear as I re-read the original comment.

>>
>> The first thing that came to mind that might be close to what's being
>> done here was domain interface script, where when we print out the name
>> we Escape whatever is in script path.
>>
>> Looking at the src/util/virtconf.c for VIR_CONF_STRING and
>> virConfParseString would seem to imply to me the ability to read escaped
>> characters.
>>
>> The difference here is that you're now formatting the name; whereas,
>> previously the formatting was entirely left to whomever edited
>> /etc/libvirt/libvirtd.conf.
>>
> 
> What difference? I put some garbage into the config via vim's controlling

semantics...  where 'difference' means prior to your changes someone
would have to either edit or append libvirtd.conf in order to
alter/define the name. With these changes the name (while running) is
altered to some file which if it has unprintable characters in the name,
how would it look?

> sequences and when I used 'cat' on the config, what I got was an unescaped
> garbage, not an escaped version of what I put there, so in that aspect the
> behaviour is the same, user provides us with garbage, and in turn, he gets the
> same garbage back, there shouldn't be any compatibility or functioning issues
> connected to that.
> 
>> In the long run, recent memory says whenever there's a user provided
>> character string - the review comments have always been be sure to
>> Escape it.
>>
>> Searching for Escape in tools/*.c found a couple more "output" files.
> 
> I looked at those and as I said, those are XML escaping routines and shell
> escaping routines present mainly for reasons I mentioned above.
> 

Fair enough - it's been considered... I think what I was going at there
though was how virsh-snapshot handles output file names where the
'file=' or 'snapshot=' output is Escaped.

John