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

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

>>> On 8/11/2015 at 07:26 PM, in message
<20150811112655.GE7460@xxxxxxxxxxxxxxxxxxxxx>, Wei Liu <wei.liu2@xxxxxxxxxx>
wrote: 
> On Mon, Aug 10, 2015 at 06:35:23PM +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@xxxxxxxx> 
> >  
> > --- 
> > Changes: 
> >   - read one more byte to check bigger size problem. 
> >  
> >  tools/libxl/libxl_internal.h |  2 ++ 
> >  tools/libxl/libxl_utils.c    | 51 
> > ++++++++++++++++++++++++++++++++++---------- 
> >  2 files changed, 42 insertions(+), 11 deletions(-) 
> >  
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h 
> > index 6013628..f98f089 100644 
> > --- a/tools/libxl/libxl_internal.h 
> > +++ b/tools/libxl/libxl_internal.h 
> > @@ -4001,6 +4001,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); 
>  
> Indentation looks wrong. 
>  
> >   
> >  /* 
> >   * Local variables: 
> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c 
> > index bfc9699..9234efb 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 tolerate_shrinking_file) 
> > +{ 
> >      GC_INIT(ctx); 
> >      FILE *f = 0; 
> >      uint8_t *data = 0; 
> > @@ -359,20 +361,34 @@ int libxl_read_file_contents(libxl_ctx *ctx, const  
> char *filename, 
> >      datalen = stab.st_size; 
> >   
> >      if (stab.st_size && data_r) { 
> > -        data = malloc(datalen); 
> > +        data = malloc(datalen + 1); 
> >          if (!data) goto xe; 
> >   
> > -        rs = fread(data, 1, datalen, f); 
> > -        if (rs != datalen) { 
> > -            if (ferror(f)) 
> > +        rs = fread(data, 1, datalen + 1, f); 
> > +        if (rs > datalen) { 
> > +            LOG(ERROR, "%s increased size while we were reading it", 
> > +                filename); 
> > +            goto xe; 
> > +        } 
> > + 
> > +        if (rs < datalen) { 
> > +            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); 
> > -            else 
> > +                goto xe; 
> > +            } else if (feof(f)) { 
> > +                if (tolerate_shrinking_file) { 
> > +                    datalen = rs; 
> > +                } else { 
> > +                    LOG(ERROR, "%s shrunk size while we were reading it", 
> > +                        filename); 
> > +                    goto xe; 
> > +                } 
> > +            } else { 
> >                  abort(); 
> > -            goto xe; 
> > +            } 
>  
> This is a bit bikeshedding, but you can leave "goto xe" out of two `if' 
> to reduce patch size. 

I guess you mean if (ferror(f)) and if (feof(f)) ? We can't leave 'goto xe' 
outside,
since in if (feof(f)) && if (tolerate_shrinking_file), it's not error but an 
expected
result in sysfs case.   

> >          } 
> > + 
> > +        data = realloc(data, datalen); 
>  
> Should check return value of realloc.

Will add a check:
if (!data) goto xe; 

Thanks,
Chunyan
>  
> The logic of this function reflects what has been discussed so far. 
>  
> Wei. 
>  
> >      } 
> >   
> >      if (fclose(f)) { 
> > @@ -396,6 +412,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,                 \ 
> > --  
> > 2.1.4 
>  
>  


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