Re: [PATCH v2 3/5] add slurp_proc_file()

[Sven Schnelle <svens@linux.ibm.com>]

Philipp Rudo <prudo@redhat.com> writes:

> On Wed, 15 Dec 2021 11:18:34 +0100
> Sven Schnelle <svens@linux.ibm.com> wrote:
>
>> slurp_file() cannot be used to read proc files, as they are returning
>> a size of zero in stat(). Add a function slurp_proc_file() which is
>> similar to slurp_file(), but doesn't require the size of the file to
>> be known.
>> 
>> Signed-off-by: Sven Schnelle <svens@linux.ibm.com>
>> ---
>>  kexec/kexec.c | 32 ++++++++++++++++++++++++++++++++
>>  1 file changed, 32 insertions(+)
>> 
>> diff --git a/kexec/kexec.c b/kexec/kexec.c
>> index f63b36b771eb..a1acba2adf2a 100644
>> --- a/kexec/kexec.c
>> +++ b/kexec/kexec.c
>> @@ -1106,6 +1106,38 @@ static void remove_parameter(char *line, const char *param_name)
>>  	}
>>  }
>>  
>> +static char *slurp_proc_file(const char *filename, size_t *len)
>> +{
>> +	ssize_t ret, startpos = 0;
>> +	unsigned int size = 64;
>> +	char *buf = NULL, *tmp;
>> +	int fd;
>> +
>> +	fd = open(filename, O_RDONLY);
>> +	if (fd == -1)
>> +		return NULL;
>> +
>> +	do {
>> +		size *= 2;
>> +		tmp = realloc(buf, size);
>> +		if (!tmp) {
>> +			free(buf);
>> +			return NULL;
>> +		}
>> +		buf = tmp;
>> +
>> +		ret = read(fd, buf + startpos, size - startpos);
>> +		if (ret < 0) {
>> +			free(buf);
>> +			return NULL;
>> +		}
>> +		startpos += ret;
>> +		*len = startpos;
>> +	} while (ret == size);
>
> I don't think this will work as intended. 
>
> 1) read returns the bytes read. So ret has a maximum value of
>    size - startpos and thus can only be equal to size on the first pass
>    when startpos = 0.
>
> 2) it's not an error when read reads less bytes than requested but can
>    happen when, e.g. it get's interrupted.
>
> The simplest solution I see is to simply use 'while (ret)' Even when
> that means that there is an extra pass in the loop with an extra
> call to realloc.
>
> The cleaner solution probably would be to put the read into a second
> loop. I.e. the following should work (no tested)
>
> 	do {
> 		[...]
> 		do {
> 			ret = read(fd, buf + startpos, size - startpos);
> 			if (ret < 0) {
> 				free(buf);
> 				return NULL;
> 			}
> 			startpos += ret;
> 		while (ret && startpos != size);
>
> 		*len = startpos;
> 	} while (ret);
>
> Thanks
> Philipp

Sigh. True. I'll prepare a v3...

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec