Re: [RFC 02/19] xen: guestcopy: Provide an helper to copy string from guest

[Julien Grall <julien.grall@linaro.org>]

On 06/18/2014 01:49 PM, Jan Beulich wrote:
>>>> On 18.06.14 at 14:22, <julien.grall@linaro.org> wrote:
>> On 06/18/2014 12:59 PM, Jan Beulich wrote:
>>>>>> On 18.06.14 at 00:43, <dgdegra@tycho.nsa.gov> wrote:
>>>> On 06/17/2014 05:23 AM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 17/06/14 10:17, Jan Beulich wrote:
>>>>>>>>> On 17.06.14 at 11:09, <julien.grall@linaro.org> wrote:
>>>>>>> On 17/06/14 09:01, Jan Beulich wrote:
>>>>>>>>>>> On 16.06.14 at 18:17, <julien.grall@linaro.org> wrote:
>>>>>>>>> +
>>>>>>>>> +    /* Add an extra +1 to append \0. We can't assume the guest will
>>>>>>>>> +     * provide a valid string */
>>>>>>>>
>>>>>>>> Now this is the case for flask, but for a generic string copying
>>>>>>>> routine I don't think this is desirable. It seems especially wrong to
>>>>>>>> aid the guest with putting a NUL where none was. If you really
>>>>>>>> want this, I guess you would be better off adding two variants:
>>>>>>>> One which demands the string to be NUL-terminated (in which
>>>>>>>> case passing in a size is sort of bogus), and one which takes a
>>>>>>>> size and inserts a NUL.
>>>>
>>>> I'm not sure why you would want a string copy-in function to not
>>>> NUL-terminate the strings it copies in.  If you don't want the strings
>>>> to be NUL-terminated at all, I would call it buffer copy-in function
>>>> (and copy_from_guest seems to cover buffer copy-in better).  If you want
>>>> the strings to be NUL-terminated and the guest has passed you a length,
>>>> it's simpler to have the hypervisor add the NUL instead of copying it
>>>> and then checking that it is there.  The current toolstack code for
>>>> XSM/FLASK relies on the hypervisor to add the NUL terminator, since it
>>>> often passes in (s, strlen(s)).
>>>
>>> I didn't say to just leave such strings unterminated. Instead I said
>>> that if there is no zero terminator, rather than putting one there we
>>> should just fail the operation if the buffer size limit was exceeded.
>>
>> It looks like I use the same trick as for flask, i.e using strlen(s) and
>> therefore let the hypervisor set the NUL-terminator.
>>
>> I will add a comment on this function to say that we expect the
>> hypervisor to set the NUL-terminator.
> 
> But just to make sure - the generic helper introduced there shouldn't
> behave that way if being given the proposed name.

How will you rename the function?

-- 
Julien Grall