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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Chunyan Liu writes ("[Xen-devel] [PATCH V4 2/7] libxl_read_file_contents: add 
new entry to read sysfs file"):
> 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.
> 
> Add a new entry libxl_read_sysfs_file_contents to handle sysfs file
> specially. It would be used in later pvusb work.

I think this patch is roughly right, but:

> -int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> -                             void **data_r, int *datalen_r) {
> +static int libxl_read_file_contents_core(libxl_ctx *ctx, const char 
> *filename,
> +                                         void **data_r, int *datalen_r,
> +                                         bool is_sysfs_file)

I would prefer a functional rather than contextual name for the
variabvle is_sysfs_file.  How about `tolerate_shrinking_file' ?

> @@ -360,15 +362,16 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char 
> *filename,
>  
>      if (stab.st_size && data_r) {
>          data = malloc(datalen);
> +        memset(data, 0, datalen);

What is this for ?

>          if (!data) goto xe;
>  
>          rs = fread(data, 1, datalen, f);
> -        if (rs != datalen) {
> +        if (rs != datalen && !(feof(f) && is_sysfs_file)) {
>              if (ferror(f))
>                  LOGE(ERROR, "failed to read %s", filename);
>              else if (feof(f))

I think it would be better to handle the special case here, with
something like
   if (!tolerate_shrinking_file)
       error stuff
   else {
       assert(datalen_r);
and to move the `goto xe' into the individual branches.


Is there any risk that the file is actually bigger than advertised,
rather than smaller ?

> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index 1c1761d..7639662 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
...
> +int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> +                                   void **data_r, int *datalen_r);

I think this is a function with sufficiently odd semantics, and a
sufficiently internal purpose, that it should probably not be exposed
in the API.

Ian.

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