Trevor Thompson

Not without doing some work, or copying the .Buffer to a PWSTR. It looks like NtfsOpenFile() passes the variable on to several functions that expect a PWSTR. What is the issue with leaving it as a ...

Not without doing some work, or copying the .Buffer to a PWSTR. It looks like NtfsOpenFile() passes the variable on to several functions that expect a PWSTR. What is the issue with leaving it as a PWSTR? That maybe the buffer won't be null-terminated? I can see that FullPath.Buffer will be, I'm not sure about FileObject->FileName.Buffer.

It should have been documented but I thought I could be lazy for such a small function. I fixed it now https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

It should have been documented but I thought I could be lazy for such a small function. I fixed it now

I see that rxprocs.h does indeed define QuadAlign() this way, but our cdfs and udfs drivers define it differently, using four bytes (which actually makes more sense to me - Quad = 4). What's up wit...

I see that rxprocs.h does indeed define QuadAlign() this way, but our cdfs and udfs drivers define it differently, using four bytes (which actually makes more sense to me - Quad = 4). What's up with that?

I think I prefer adding something like this to ntfs.h:
#define ATTR_RECORD_ALIGNMENT 8 /* The beginning and length of an attribute-record are always aligned to an 8-byte boundary, relative to the beginning of the file record. */

Thanks! My local copy is now totally int-free everywhere throughout the project. https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

Thanks! My local copy is now totally int-free everywhere throughout the project.

I'm not sure which insane thing you're referring to. The STATUS_PENDING return? The tons of parameters? The fact that UpdateFileNameRecord() wasn't even checking for STATUS_PENDING so changes to in...

I'm not sure which insane thing you're referring to. The STATUS_PENDING return? The tons of parameters? The fact that UpdateFileNameRecord() wasn't even checking for STATUS_PENDING so changes to index root wouldn't get written to disk?

In any case, my answer would be, "It's not very sane." I should probably rewrite this function when I get a chance, now that I understand indices better. In the meantime, I've fixed UpdateFileNameRecord().

That makes sense to me and passes all of my tests, so I'll go ahead an change it. Maybe Pierre can chime in to be sure - my contribution to this line was changing the ) to a comma. https://code.rea...

That makes sense to me and passes all of my tests, so I'll go ahead an change it. Maybe Pierre can chime in to be sure - my contribution to this line was changing the ) to a comma.

In this case, using DbgPrint() was intentional, because I want to convey the layout of the tree visually without having line numbers get in the way. All the Dump...() functions in the driver work l...

In this case, using DbgPrint() was intentional, because I want to convey the layout of the tree visually without having line numbers get in the way. All the Dump...() functions in the driver work like this.

An attribute record header is always aligned to an 8-byte boundary. That's just the way Microsoft made it; I have no way to know their reasoning. See Forensic Computing: A Practitioner's Guide, Sec...

An attribute record header is always aligned to an 8-byte boundary. That's just the way Microsoft made it; I have no way to know their reasoning. See Forensic Computing: A Practitioner's Guide, Second Edition, p. 393-394.

After using this script for a while, I think I favor exiting with an error here. Displaying the available options would be nice too.

After using this script for a while, I think I favor exiting with an error here. Displaying the available options would be nice too.

This is broken. Should be more like: FsRtlDissectName(Remaining, &Current, &Remaining); FirstEntry = 0;

This is broken. Should be more like:
FsRtlDissectName(Remaining, &Current, &Remaining);
FirstEntry = 0;

This needs to go for iteration 3; it can hide an error from the client. I'm also going to get rid of the RealLengthWritten variable altogether. I initially had WriteAttribute() indicating the amou...

This needs to go for iteration 3; it can hide an error from the client. I'm also going to get rid of the RealLengthWritten variable altogether.

I initially had WriteAttribute() indicating the amount of data actually written to the disk (a multiple of the sector size) but I later decided this didn't make sense and changed it. However, I didn't update NtfsWriteFile() to reflect that decision.

I cannot ... rely on anything from Cc? We definitely don't use the cache yet https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif I just didn't know who was respons...

I cannot ... rely on anything from Cc?

We definitely don't use the cache yet I just didn't know who was responsible for freeing the MDL that we could be allocating in the call to NtfsLockUserBuffer. My comments in both revisions are probably too focused on whose job it was to free that MDL; it was mostly an academic curiousity*.

What I was really getting at, is I thought it was strange that this code could be responsible for allocating and locking the MDL, but that I would get a bugcheck if I tried to free the MDL myself (though it makes sense to me now). Sorry that neither of my revisions said that clearly.

My main concern was in documenting that it is not an error that we don't free the MDL, even though we may have allocated it.
"Using MDL's" https://msdn.microsoft.com/en-us/library/windows/hardware/ff565421%28v=vs.85%29.aspx was one of many resources I had time to skim but not read in-depth while preparing the first patch in time for GSoC's submission deadline. I missed this before, which explained everything to me: "When the IRP is completed, the system unlocks and frees all the MDLs that are associated with the IRP." I'll update this comment to say that.

*P.s. I now know it's the I/O manager "When the I/O manager completes an I/O request, the I/O manager frees the IRP and frees any MDLs that are attached to the IRP. Some of these MDLs might have been attached to the IRP by drivers that are located beneath the I/O manager in the device stack."

Yep! https://code.reactos.org/static/olpro3/2static/images/wiki/icons/emoticons/smile.gif

Yep!

That code is in the trunk. The problem is really just my inexperience with SVN and Jira. For the first patch, I erred on the side of submitting every change I made. For the second patch, I did the ...

That code is in the trunk. The problem is really just my inexperience with SVN and Jira. For the first patch, I erred on the side of submitting every change I made. For the second patch, I did the opposite, not realizing the parts I didn't patch would be showing up as changes for iteration 2 vs iteration 1. Thanks for your patience!

And I agree, it looks really weird to me to. I just figured there was some reason we were doing it that way, shrugged and added it back. It's gone for iteration 3

This is where I'm totally lost. Are your referring to one of those three cases in particular? By allocation size, do you mean Fcb->RFCB.AllocationSize? I thought this value represents the size all...

This is where I'm totally lost. Are your referring to one of those three cases in particular?

By allocation size, do you mean Fcb->RFCB.AllocationSize? I thought this value represents the size allocated for the file on the disk. This code doesn't create a new file stream, it just reuses the existing stream. Why (and when) is it better to set this allocation size to 0 instead of leaving it as the actual value?

Are you referring to something else when you say allocation size?

Can you please explain what you mean, or point me toward the right resource that can explain it?

This if() shouldn't be here.

This if() shouldn't be here.

I'll try taking it out for the next revision. It's just vestigial code from NtfsReadDisk(). Sorry I didn't leave a comment saying that. Edit: It's not from VFat like I said earlier.

I'll try taking it out for the next revision. It's just vestigial code from NtfsReadDisk(). Sorry I didn't leave a comment saying that.

Edit: It's not from VFat like I said earlier.

Since I wasn't trying to offer support to extend files with this patch, doing it this way made testing a little bit easier on me. To be honest, it's meant to be temporary code, so I didn't think m...

Since I wasn't trying to offer support to extend files with this patch, doing it this way made testing a little bit easier on me.

To be honest, it's meant to be temporary code, so I didn't think much about it, but I see your point. The next iteration will not report success here.

This is by design. My aim for this patch was to make the minimum testable progress toward write support (and build on it), not to provide full overwrite functionality. Updating a file's size is st...

This is by design. My aim for this patch was to make the minimum testable progress toward write support (and build on it), not to provide full overwrite functionality.

Updating a file's size is still TODO, but reusing the existing data run is how I was planning to handle overwriting a file if the cluster allocations don't need to change.

Can you please elaborate on why this wouldn't work fine?

That line I definitely didn't mean to leave in the patch!

That line I definitely didn't mean to leave in the patch!

It doesn't fix any problems that I can recall. To be honest, I don't really remember my motivation for that. A dubious optimization? I'm not sure I meant to submit that in the patch. I'll try takin...

It doesn't fix any problems that I can recall. To be honest, I don't really remember my motivation for that. A dubious optimization? I'm not sure I meant to submit that in the patch. I'll try taking it out for the next revision.