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]

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.

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

>   +    {
>  +        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?

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