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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

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
possibility.


[...]
@@ -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",
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfilename);
> +ÂÂÂÂÂÂÂÂÂÂÂÂgoto xe;
> +ÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂ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
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto xe;
> +ÂÂÂÂÂÂÂÂÂÂÂÂ} else if (feof(f)) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (tolerate_shrinking_file) {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdatalen = rs;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ} else {
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂLOG(ERROR, "%s shrunk size while we were reading
> it",
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfilename);
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgoto xe;
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> +ÂÂÂÂÂÂÂÂÂÂÂÂ} else {
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂabort();
> -ÂÂÂÂÂÂÂÂÂÂÂÂgoto xe;
> +ÂÂÂÂÂÂÂÂÂÂÂÂ}
> ÂÂÂÂÂÂÂÂÂ}
> +
> +ÂÂÂÂÂÂÂÂ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,
> +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂvoid **data_r, int *datalen_r)
> +{
> +ÂÂÂÂ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.

Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel