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

["Chun Yan Liu" <cyliu@xxxxxxxx>]

>>> On 6/12/2015 at 12:16 AM, in message
<21881.46148.466883.923039@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 
>  
>  



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