Re: [ros-dev] [ros-diffs] [tthompson] 71820: [NTFS] Add ability to write to resident attributes. SetAttributeDataLength() - Check if the file is memory mapped before truncating +InternalSetResidentAttributeLength() - Used b...

[Pierre Schweitzer]

Le 06/07/2016 19:59, Trevor Thompson a écrit :
>   Hi Pierre, thanks for the fast review! I'll
> respond inline.
>  
>  On Tue, Jul 5, 2016 at 4:39 PM, Pierre Schweitzer <pierre(a)reactos.org>
>  wrote:
>  
> >   Hi,
> >
> >  Comments inline.
> >
> >  Le 05/07/2016 09:00, tthompson(a)svn.reactos.org a écrit :
> >  ==============================================================================
> > >   ---
> > > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c 
> >  [iso-8859-1] (original)
> > >   +++
> > > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c 
> >  [iso-8859-1] Tue Jul  5
> > 07:00:43 2016
> > >   @@ -158,7 +157,7 @@
> > >       DPRINT1("Total clusters in bitmap: %I64x\n", BitmapDataSize * 8);
> > >       DPRINT1("Diff in size: %I64d B\n", ((BitmapDataSize * 8) - 
> > DeviceExt->NtfsInfo.ClusterCount) * DeviceExt->NtfsInfo.SectorsPerCluster *
> >  DeviceExt->NtfsInfo.BytesPerSector);
> > >  
> > >  -    ReadAttribute(DeviceExt, DataContext, Read, 
> >  (PCHAR)((ULONG_PTR)BitmapData +
> > Read), (ULONG)BitmapDataSize);
> > >   +    ReadAttribute(DeviceExt, DataContext, 0,
> > > (PCHAR)BitmapData, 
> >  (ULONG)BitmapDataSize);
> >
> >  What's this change?
> >  
>  
>  I'm getting rid of the Read variable, which isn't being used in any
>  meaningful way. This just removes all references to said variable. It was
>  just vestigial from this bit of code I copy-pasted from
>  NtfsGetFreeClusters():
>  
>  /* FIXME: Totally underoptimized! */
>      for (; Read < BitmapDataSize; Read +=
>  DeviceExt->NtfsInfo.BytesPerSector)
>      {
>          ReadAttribute(DeviceExt, DataContext, Read,
>  (PCHAR)((ULONG_PTR)BitmapData + Read), DeviceExt->NtfsInfo.BytesPerSector);
>      }
>  
>  By the way, is there any reason I shouldn't heed the comment and replace
>  this with a single call to ReadAttribute()? 
I don't recall why I had to do it that way... Perhaps I was hitting a
bug somewhere else (perhaps read alignment issues?). But if it doesn't
regress anything, just feel free to do it properly.

-- 
Pierre Schweitzer <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.