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...

[Trevor Thompson]

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 :
> >   Modified:
> > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c
> >  URL: 
> http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst…
> >   
> ==============================================================================
> >   ---
> > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c 
>  [iso-8859-1] (original)
> >   +++
> > branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/mft.c 
>  [iso-8859-1] Tue Jul  5
> 07:00:43 2016
>  > @@ -170,6 +172,50 @@
>  >          return AttrRecord->Resident.ValueLength;
>  >  }
> >   
>  > +void
>  > +InternalSetResidentAttributeLength(PNTFS_ATTR_CONTEXT AttrContext,
>  > +                                   PFILE_RECORD_HEADER FileRecord,
>  > +                                   ULONG AttrOffset,
>  > +                                   ULONG DataSize)
>  > +{
>  > +    ULONG EndMarker = AttributeEnd;
>  > +    ULONG FinalMarker = FILE_RECORD_END;
>  > +    ULONG NextAttributeOffset;
>  > +    ULONG Offset;
>  > +    USHORT Padding;
>  > +
>  > +    DPRINT("InternalSetResidentAttributeLength( %p, %p, %lu, %lu )\n",
>  AttrContext, FileRecord, AttrOffset, DataSize);
> >   +
> >  +    // update ValueLength Field
> >  +    AttrContext->Record.Resident.ValueLength = DataSize;
> >  +    Offset = AttrOffset + FIELD_OFFSET(NTFS_ATTR_RECORD, 
>  Resident.ValueLength);
> >   +    RtlCopyMemory((PCHAR)FileRecord + Offset,
> > &DataSize, sizeof(ULONG)); 
>  That part looks a bit over-engineered to me and might be simplified
>  (like not using an intermediate var, nor RtlCopyMemory).
>  Cosmetic, I'd replace PCHAR by ULONG_PTR.
>  Same comment goes to code below doing basically the same thing.
>  
That's a good point. The code sort of just evolved that way; I can instead
get a pointer to the target record and access its fields directly.


> >   +
> >  +    // calculate the record length and end marker offset
> >  +    AttrContext->Record.Length = DataSize + 
> AttrContext->Record.Resident.ValueOffset;
> >   +    NextAttributeOffset = AttrOffset +
> > AttrContext->Record.Length;
> >  +
> >  +    // Ensure NextAttributeOffset is aligned to an 8-byte boundary
> >  +    if (NextAttributeOffset % 8 != 0)
> >  +    {
> >  +        Padding = 8 - (NextAttributeOffset % 8);
> >  +        NextAttributeOffset += Padding;
> >  +        AttrContext->Record.Length += Padding;
> >  +    }
> >  +
> >  +    // update the record length
> >  +    Offset = AttrOffset + FIELD_OFFSET(NTFS_ATTR_RECORD, Length);
> >  +    RtlCopyMemory((PCHAR)FileRecord + Offset, 
>  &AttrContext->Record.Length,
> sizeof(ULONG));
> >   +
> >  +    // write the end marker
> >  +    RtlCopyMemory((PCHAR)FileRecord + NextAttributeOffset, &EndMarker, 
> sizeof(ULONG));
> >   +
> >  +    // write the final marker
> >  +    Offset = NextAttributeOffset + sizeof(ULONG);
> >  +    RtlCopyMemory((PCHAR)FileRecord + Offset, &FinalMarker, 
>  sizeof(ULONG));
> >   +
> >  +    FileRecord->BytesInUse = Offset + sizeof(ULONG);
> >  +}
> >
> >   NTSTATUS
> >   SetAttributeDataLength(PFILE_OBJECT FileObject,
> >  @@ -179,6 +225,18 @@
> >                          PFILE_RECORD_HEADER FileRecord,
> >                          PLARGE_INTEGER DataSize)
> >   {
> >  +    NTSTATUS Status = STATUS_SUCCESS;
> >  +
> >  +    // are we truncating the file?
> >  +    if (DataSize->QuadPart < AttributeDataLength(&AttrContext->Record)
>  Unless I'm blind, I'm not sure it builds.
>  
For shame! I really don't know how that happened, sorry :( It's been fixed.


>  
> >   +    {
> >  +        if (!MmCanFileBeTruncated(FileObject->SectionObjectPointer, 
> (PLARGE_INTEGER)&AllocationSize))
> >   +        {
> >  +            DPRINT1("Can't truncate a memory-mapped file!\n");
> >  +            return STATUS_USER_MAPPED_FILE;
> >  +        }
> >  +    }
> >  +
> >       if (AttrContext->Record.IsNonResident)
> >       {
> >           ULONG BytesPerCluster = Fcb->Vcb->NtfsInfo.BytesPerCluster;
> >  Modified: branches/GSoC_2016/NTFS/drivers/filesystems/ntfs/volinfo.c
> >  URL: 
> http://svn.reactos.org/svn/reactos/branches/GSoC_2016/NTFS/drivers/filesyst…
> >   
> ==============================================================================
> >   ---
> > 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()?


> >  
> >       RtlInitializeBitMap(&Bitmap, (PULONG)BitmapData, 
> DeviceExt->NtfsInfo.ClusterCount);
> >        FreeClusters =
> > RtlNumberOfClearBits(&Bitmap);
> >
> >  
>
>  --
>  Pierre Schweitzer <pierre at reactos.org>
>  System & Network Administrator
>  Senior Kernel Developer
>  ReactOS Deutschland e.V.
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>