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

2015-06-12 Thread Chun Yan Liu 


 On 6/12/2015 at 12:16 AM, in message
21881.46148.466883.923...@mariner.uk.xensource.com, Ian Jackson
ian.jack...@eu.citrix.com wrote: 
 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' ? 

OK.

  
  @@ -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 ? 

I found sometimes reading sysfs file contents, at the end, there is
some random character. With this line, there is no problem then.

  
   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. 

OK. Will update.

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

For sysfs file, couldn't be bigger.

  
  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. 

So move to libxl_internal.h? Or not hacking here but just adding an internal
function in libxl_pvusb.c with repeated codes?

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



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

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

2015-06-12 Thread Ian Jackson 
Chun Yan Liu writes (Re: [Xen-devel] [PATCH V4 2/7] libxl_read_file_contents: 
add new entryto read sysfs file):
 21881.46148.466883.923...@mariner.uk.xensource.com, Ian Jackson
 ian.jack...@eu.citrix.com wrote: 
  Chunyan Liu writes ([Xen-devel] [PATCH V4 2/7] libxl_read_file_contents: 
  add  
  What is this for ? 
 
 I found sometimes reading sysfs file contents, at the end, there is
 some random character. With this line, there is no problem then.

I'm afraid that's not really a complete explanation.

Guessing, I think your calling code expected a trailing nul.

If you want to make an API which is useable for such code, 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.

  Is there any risk that the file is actually bigger than advertised, 
  rather than smaller ? 
 
 For sysfs file, couldn't be bigger.

Then you should detect the condition that the file is bigger, and call
it an error.

   +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. 
 
 So move to libxl_internal.h? Or not hacking here but just adding an internal
 function in libxl_pvusb.c with repeated codes?

I meant to move it to libxl_internal.h.  Subject to consideration of
what its API ought to be.

Ian.

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

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

2015-06-11 Thread Ian Jackson 
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@lists.xen.org
http://lists.xen.org/xen-devel

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

2015-06-09 Thread Chunyan Liu 
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 cy...@suse.com

---
Changes:
  - instead of hacking lixbl_read_file_contents for sysfs file, add
a new entry libxl_read_sysfs_file_contents to do the work.

 tools/libxl/libxl_utils.c | 24 
 tools/libxl/libxl_utils.h |  2 ++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index f6be2d7..d0d56c2 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,
+ void **data_r, int *datalen_r,
+ bool is_sysfs_file)
+{
 GC_INIT(ctx);
 FILE *f = 0;
 uint8_t *data = 0;
@@ -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);
 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))
 LOG(ERROR, %s changed size while we were reading it,
-   filename);
+   filename);
 else
 abort();
 goto xe;
@@ -396,6 +399,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);
+}
+
+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);
+}
+
+
 #define READ_WRITE_EXACTLY(rw, zero_is_eof, constdata)\
   \
   int libxl_##rw##_exactly(libxl_ctx *ctx, int fd, \
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
@@ -38,6 +38,8 @@ int libxl_is_stubdom(libxl_ctx *ctx, uint32_t domid, uint32_t 
*target_domid);
 int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name);
 int libxl_string_to_backend(libxl_ctx *ctx, char *s, libxl_disk_backend 
*backend);
 
+int libxl_read_sysfs_file_contents(libxl_ctx *ctx, const char *filename,
+   void **data_r, int *datalen_r);
 int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
  void **data_r, int *datalen_r);
   /* Reads the contents of the plain file filename into a mallocd
-- 
2.1.4


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