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

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

>>> On 6/26/2015 at 09:05 PM, in message
<21901.20009.85407.581628@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson
<Ian.Jackson@xxxxxxxxxxxxx> wrote: 
> Chun Yan Liu writes ("Re: [PATCH V5 2/7] libxl_read_file_contents: add new  
> entry to read sysfs file"): 
> > >>> On 6/25/2015 at 07:09 PM, in message 
> > <21899.57676.368102.982820@xxxxxxxxxxxxxxxxxxxxxxxx>, Ian Jackson 
> > <Ian.Jackson@xxxxxxxxxxxxx> wrote:  
> > > Chunyan Liu writes ("[PATCH V5 2/7] libxl_read_file_contents: add new  
> entry> > > 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 still fails to detect a situation where the file is  
> > > unexpectedly longer than the requested size ?  
> >  
> > +            } else if (feof(f)) { 
> > +                if (rs < datalen && tolerate_shrinking_file) { 
> > +                    datalen = rs; 
> > +                } else { 
> >  
> > If the file is bigger than the requested size, it will fall to this 
> > branch and report error. 
>  
> I don't think this is true.  I just applied your patch to my copy of 
> staging to see what the code looks like and saw this: 
>  
>     if (stab.st_size && data_r) { 
>         data = malloc(datalen); 
>         if (!data) goto xe; 
>  
>         rs = fread(data, 1, datalen, f); 
>         if (rs != datalen) { 
>             if (ferror(f)) { 
>                 LOGE(ERROR, "failed to read %s", filename); 
>                 goto xe; 
>             } else if (feof(f)) { 
>                 if (rs < datalen && tolerate_shrinking_file) { 
>                     datalen = rs; 
>                 } else { 
>                     LOG(ERROR, "%s changed size while we were reading it", 
>                         filename); 
>                     goto xe; 
>                 } 
>             } else { 
>                 abort(); 
>             } 
>         } 
>     } 
>  
> So I think in the case of a sysfs file which is >4096 bytes: 
>  
>  - stat will succeed and give st_size == 4096 
>  - fread(,,4096,) will read the first 4096 bytes and return 4096 
>  - rs == datalen, so we don't take the `errors and special 
>      cases' branch 
>  - we return success having read 4096 bytes 
>  
> But please feel free to explain why I'm wrong. 

You are right. I'm confused about the original logic. The original code
doesn't consider this case (size bigger than requested) at all. So, we need
to add a check if (rs == datalen && read end of file), if not, means bigger
than requested, report error.

- Chunyan

>  
> Thanks, 
> Ian. 
>  
>  



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