Re: [PATCH V7 2/7] libxl_read_file_contents: add new entry to read sysfs file

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-09-25 at 10:11 +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.
> 
> Add a new entry libxl_read_sysfs_file_contents to handle sysfs file
> specially. It would be used in later pvusb work.
> 
> Signed-off-by: Chunyan Liu <cyliu@suse.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>

I'm afraid I have one issue with this patch, sorry.

> @@ -4015,6 +4015,8 @@ void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
>  
>  int libxl__count_physical_sockets(libxl__gc *gc, int *sockets);
>  #endif
> +_hidden int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> +                                   void **data_r, int *datalen_r);

As an internal function this should be called libxl__foo (double
underscore) and take a libxl__gc *gc not libxl_ctx *ctx.

This will require some other subtle adjustments which I'll outline below.

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index bfc9699..72f8f4f 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -322,8 +322,10 @@ out:
>      return rc;
>  }
>  
> -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,

Being static this can now get a shorter name. Perhaps just
read_file_contents_core (or even drop the _core). It should also take a gc
instead of a ctx, since it will be used by both internal and external
facing wrappers.

> +                                         void **data_r, int *datalen_r,
> +                                         bool tolerate_shrinking_file)
> +{
>      GC_INIT(ctx);

Having changed the function to take a gc you will instead want to do this
GC_INIT(ctx) in the libxl_read_file_contents public interface wrapper,
which will then give you the gc to pass here.

You will likewise need to do the GC_FREE in that wrapper too instead of
here.

@@ -396,6 +413,19 @@ int libxl_read_file_contents(libxl_ctx *ctx, const
> char *filename,
>      return e;
>  }
>  
> +int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
> +                             void **data_r, int *datalen_r)
> +{
> +    return libxl_read_file_contents_core(ctx, filename, data_r,
> datalen_r, 0);

As above this will need the GC_INIT and GC_FREE stuff doing here.

libxl_cpupool_info() looks like a good template to follow.

> +}
> +
> +int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
> +                                   void **data_r, int *datalen_r)
> +{
> +    return libxl_read_file_contents_core(ctx, filename, data_r,
> datalen_r, 1);

While this should take a gc and can just pass it on.

Ian.