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

[John Ferlan]

On 12/13/2016 11:43 AM, Erik Skultety wrote:
> On Tue, Dec 13, 2016 at 09:50:49AM -0500, John Ferlan wrote:
>>
>>
>> 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.
> 
> No, it was perfectly clear that you meant just the filename, but it doesn't
> contradict my point anyway.
> 
>>
>>>>
>>>> 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?
>>
> 
> Ugly. Still, they can put a name containing unprintable characters to the
> config and the daemon will create the file on the filesystem. After my changes,
> the same logic applies. IOW the user is responsible for the formatting of the
> filename in both cases.
> 
>>> 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.
>>
> 
> I understand, XMLs are special in this way and since since commit @aeb5262e we
> strip the control characters completely anyway so even our existing 'escaping'
> routine for XMLs isn't a proper one.
> 
> Soo, did we reach a consensus?
> 
> Erik
> 

The only thing holding up this patch would be your update to
tools/virt-admin.pod to describe the name of the command (there was no
daemon-log-info). Like I said the escaping was only a question of
whether it was necessary - I think you've answered that.

John