[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH V8 2/7] libxl_read_file_contents: add new entry to read sysfs file

On Wed, 2015-10-21 at 17:08 +0800, Chunyan Liu wrote:
> Sysfs file has size=4096 but actual file content is less than that.
> Current libxl_read_file_contents will treat it as error when file size
> and actual file content differs, so reading sysfs file content with
> this function always fails.

You changes to the existing code seem to do more than just cope with this

@@ -359,20 +360,35 @@ int libxl_read_file_contents(libxl_ctx *ctx, const
> char *filename,
> ÂÂÂÂÂdatalen = stab.st_size;
> Â
> ÂÂÂÂÂif (stab.st_size && data_r) {
> -ÂÂÂÂÂÂÂÂdata = malloc(datalen);
> +ÂÂÂÂÂÂÂÂdata = malloc(datalen + 1);

This (and the related change) seem to be fixing an off by one bug in the
existing code? At a minimum this should be mentioned in the commit message
but ideally it would be split out into a precursor code, so as to allow it
to be backported (assuming it is a bug fix and not some other consequence
of reading from sysfs).

> ÂÂÂÂÂÂÂÂÂif (!data) goto xe;
> Â
> -ÂÂÂÂÂÂÂÂrs = fread(data, 1, datalen, f);
> -ÂÂÂÂÂÂÂÂif (rs != datalen) {
> -ÂÂÂÂÂÂÂÂÂÂÂÂif (ferror(f))
> +ÂÂÂÂÂÂÂÂrs = fread(data, 1, datalen + 1, f);
> +ÂÂÂÂÂÂÂÂif (rs > datalen) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂLOG(ERROR, "%s increased size while we were reading it",
> +
> +ÂÂÂÂÂÂÂÂif (rs < datalen) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂif (ferror(f)) {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂLOGE(ERROR, "failed to read %s", filename);
> -ÂÂÂÂÂÂÂÂÂÂÂÂelse if (feof(f))
> -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂLOG(ERROR, "%s changed size while we were reading it",
> -             ÂÂÂÂfilename);
> +ÂÂÂÂÂÂÂÂÂÂÂÂ} else if (feof(f)) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (tolerate_shrinking_file) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂLOG(ERROR, "%s shrunk size while we were reading
> it",
> +
> +ÂÂÂÂÂÂÂÂdata = realloc(data, datalen);
> +ÂÂÂÂÂÂÂÂif (!data) goto xe;

and this change I don't follow at all. Is it shrinking the buffer? I'm not
sure that is needed, it should be a smallish waste of memory.

If you feel strongly that the realloc is needed then you should call
libxl__realloc(NOGC, ...) to get the correct behaviour on error.

That NOGC brings me onto my last point:
> +int libxl__read_sysfs_file_contents(libxl__gc *gc, const char *filename,
> +{
> +ÂÂÂÂreturn read_file_contents_core(gc, filename, data_r, datalen_r, 1);

Since this is now an internal function it would normally be expected that
the returned buffer would be garbage collected.

Since you want to share the core code with an external function which
should return un-gc'd memory it seems like the easiest change would be to
call libxl__ptr_add here.

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.