Hi Pierre, thanks for the fast review! I'll respond inline.

On Tue, Jul 5, 2016 at 4:39 PM, Pierre Schweitzer <pierre@reactos.org> wrote:

Hi,

Comments inline.

Le 05/07/2016 09:00, tthompson@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/filesystems/ntfs/mft.c?rev=71820&r1=71819&r2=71820&view=diff
> ==============================================================================
> --- 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/filesystems/ntfs/volinfo.c?rev=71820&r1=71819&r2=71820&view=diff
> ==============================================================================
> --- 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@reactos.org
http://www.reactos.org/mailman/listinfo/ros-dev