Hi,
On 08/05/2016 11:39 AM, Valentine Sinitsyn wrote:
> Hi all,
>
> On 05.08.2016 14:28, Ralf Ramsauer wrote:
>> Hi Henning,
>>
>> On 08/05/2016 09:55 AM, Henning Schild wrote:
>>> Am Thu, 4 Aug 2016 21:28:14 +0200
>>> schrieb Ralf Ramsauer <[email protected]>:
>>>
>>>> Check if file size is not zero and check the return value of close()
>>>> as it might fail, though it's very unlikely.
>>>>
>>>> Signed-off-by: Ralf Ramsauer <[email protected]>
>>>> ---
>>>> tools/jailhouse.c | 10 +++++++++-
>>>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/jailhouse.c b/tools/jailhouse.c
>>>> index 5bf9b0f..8c7783c 100644
>>>> --- a/tools/jailhouse.c
>>>> +++ b/tools/jailhouse.c
>>>> @@ -156,6 +156,11 @@ static void *read_file(const char *name, size_t
>>>> *size) exit(1);
>>>> }
>>>>
>>>> + if (stat.st_size == 0) {
>>>> + fprintf(stderr, "reading empty file: %s\n", name);
>>>> + exit(1);
> This hardcode look scary. What if one want a different message or
> behavior? Wouldn't it be better to return some error code and let the
> caller decide?
You can find this kind of exit-on-fail strategy all over
tools/jailhouse.c (open_dev(), read_string(), read_file(), ...)
That's why I chose to exit in this case.
>
>>>> + }
>>>> +
>>>
>>> Being too smart about such things is - in general - not a good idea. It
>>> is usually better to try whatever you want to do and handle errors.
>>> Size > 0 checks will fail in procfs and readable size == stat.size will
>>> fail in sysfs. Similar things can be true for permission checks and
>> Current code is not suitable for procfs, as malloc() is aligned to
>> st_size. sysfs will return PAGE_SIZE as an upper boundary. Patch 4/7
>> fixes this issue by allowing read() to return smaller values as st_size.
>>> funny filesystems like fuse or cifs.
>> Didn't experience such behaviour yet - guess you did :)
>>>
>>>> buffer = malloc(stat.st_size);
>> Ok, so if we don't check and st_size is zero, we're calling malloc(0)
>> and according to the malloc manpage, it might return NULL, and the
>> produced error message "insufficient memory" is not the proper message.
> Allocating a reasonable initial amount then realloc() as you read the
> file should work.
Hm, results in looped read(). I'll think about that.
Ralf
>
> Valentine
>
>>
>> Ralf
>>>> if (!buffer) {
>>>> fprintf(stderr, "insufficient memory\n");
>>>> @@ -167,7 +172,10 @@ static void *read_file(const char *name, size_t
>>>> *size) exit(1);
>>>> }
>>>>
>>>> - close(fd);
>>>> + if (close(fd)) {
>>>> + fprintf(stderr, "closing %s: %s\n", name,
>>>> strerror(errno));
>>>> + exit(1);
>>>> + }
>>>>
>>>> if (size)
>>>> *size = stat.st_size;
>>>
>>
--
Ralf Ramsauer
PGP: 0x8F10049B
--
You received this message because you are subscribed to the Google Groups
"Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
