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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Ian Campbell writes ("Re: [PATCH V8 2/7] libxl_read_file_contents: add new 
entry to read sysfs file"):
> @@ -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).

There is no off-by-one error in the existing code.  I think this +1
arises because Chunyan wants libxl__read_sysfs_file_contents to
produce a nul-terminated string, even though libxl_read_file_contents
doesn't.

In June I wrote:

  If you want to make an API which is useable for [code which wants a
  trailing nul], and not intended for byte blocks, that's fine, but
  you must then always nul-terminate (not only if the data was less
  than 4k) and your new function probably doesn't want to return a
  length at all.

I am coming to the thought that it might be better for this to be two
functions with entirely separate implementations.  The resulting
unified function is already full of tangled conditional logic, and our
complaint here will involve yet another conditional.

Ian.

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