From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Jackson <Ian.Jackson@eu.citrix.com>
Subject: Re: [PATCH V4 2/7] libxl_read_file_contents: add new
	entry	to read sysfs file
Date: Thu, 11 Jun 2015 17:16:04 +0100
Message-ID: <21881.46148.466883.923039@mariner.uk.xensource.com>
References: <1433906441-3280-1-git-send-email-cyliu@suse.com>
	<1433906441-3280-3-git-send-email-cyliu@suse.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1433906441-3280-3-git-send-email-cyliu@suse.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Chunyan Liu <cyliu@suse.com>
Cc: george.dunlap@eu.citrix.com, wei.liu2@citrix.com, ian.campbell@citrix.com, xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.