338 lines
13 KiB
Plaintext
338 lines
13 KiB
Plaintext
This is a list of code "concerns". these have been removed from the code and
|
||
placed here so that the code will not contain untoward comments.
|
||
|
||
rx\read.c and write.c
|
||
tomm claims that the memory manager has been fixed before nt4.0 shipped
|
||
so that it is no longer necessary to synchronize flushers by acquiring and
|
||
releasing the pagingio resource. the is in the check for noncachedio when there
|
||
is a private cachemap. we should make sure that everyone else removes their
|
||
synchronization or we should put ours back.
|
||
|
||
rx\read.c and write.c
|
||
the pipe read/write paths should be completely separated in the wrapper
|
||
and the opportunity to do so should be presented in the mini.
|
||
|
||
rx\read.c
|
||
there is a place where we set status pipe empty; at that point we should
|
||
also backoff a la rdr1
|
||
|
||
rx\lock.c
|
||
there should be backoff logic
|
||
|
||
rx\strtstop.c
|
||
the call to rxforcenettablefinalization should be per-minirdr and should
|
||
be moved out of the iftest.
|
||
|
||
rx\strtstop
|
||
acquireing the netname table lock is was only required becauseof
|
||
precious servers. now that that's gone. we should remove the lock. but, it's
|
||
safe, of course.
|
||
|
||
mrx in general
|
||
we should review the throttling constants....seem pretty conservative to
|
||
me; we should consider being substantially more aggressive.
|
||
|
||
mrx
|
||
we should have precious server code.
|
||
|
||
rx\lockctrl.c
|
||
we should only allocate a lock structure if someone does a lock! same
|
||
with the oplock structure. ntfs does this. obviously, we would have a change in
|
||
fcb.h to go with this.
|
||
|
||
rx\lockctrl.c
|
||
the last little bit of getting the refcounts right was to account for
|
||
the fact that the fspdispatch does not remove a reference if pending is
|
||
returned. this is probably wrong....and we should map over to a scheme where the
|
||
fsd and fsp dispatches are the ones that do this instead of having every common
|
||
routine do it. this would be a major change but it would really make things more
|
||
consistent.
|
||
|
||
rx\lockctrl.c
|
||
it is stupid that we will have to lookup the lock again if it fails
|
||
(async completion path). we should ask for an fsrtl interface that removes a
|
||
lock that i provide.
|
||
|
||
rx\create.c
|
||
in RxCreateFromNetRoot, after the call to
|
||
|
||
rx\ntinit.c
|
||
in RxGetRegistryParameters, we should allocate space for Storage
|
||
instead. really what we should do is to implement and export a routine that both
|
||
allocates and reads a string........for Ulong no allocation should be necessary
|
||
|
||
rx\ntfsp.c
|
||
in fspdispatch and also addtoworkq, we use the wrapper's device object
|
||
for throttling instead of the minirdr's device object. this will change.
|
||
|
||
rx\ntfsp.c
|
||
in fspdispatch, if status_retry is returned then we will have already
|
||
completed the packet.
|
||
|
||
rx\ntfsp.c
|
||
prepostirp should be made pageable; also, i'm not too happy
|
||
|
||
rx\ntfsd.c
|
||
initialization of prefixtable should be in RxInitializeDispatchVectors
|
||
|
||
rx\ntfsd.c
|
||
RxFsdCommonDispatch in an excellent canididate for inlining
|
||
|
||
rx\ntfsd.c
|
||
we shouldn't be checking everywhere for null dispatch points....rather
|
||
we should fill them in with a good default
|
||
|
||
rx\ntfsd.c
|
||
we should remove the async code from everywhere else and have it only in
|
||
fsddispatch and fspdispatch. same with retry. this would get rid of the
|
||
"should i remove a reference or not" decision.
|
||
|
||
rx\??
|
||
the code that chains together pagingio reads should be removed
|
||
|
||
rx\ntfastio.c
|
||
in fastiocheckifpossible...return the error directly. also, we should
|
||
the cached netroottype as well as nodetype to determine if it's a
|
||
diskfile. also in markoncleanup.
|
||
|
||
rx\ntexcept.c
|
||
i removed this from the header
|
||
|
||
|
||
Original Comments
|
||
This routine process an exception. It either completes the request
|
||
with the saved exception status or it sends it off to IoRaiseHardError()
|
||
|
||
My New Improved Version
|
||
This routine processes an exception from the top level FSD routine. You can tell because it has
|
||
an Irp w/o an RxContext. The original code did all kinds of stuff like raising hard errors and
|
||
posting and such. My take is different...if you get here, things are hosed. So we either complete
|
||
or we bugcheck. PERIOD. we can always be more liberal later...........
|
||
|
||
rx\ntfsd.c
|
||
in RxDevFcbQueryDeviceInfo, the special handling for pipes should be
|
||
removed.
|
||
|
||
rx\lockctrl.c
|
||
don't allocate a lock structure unless you need one. also in fcb.h and
|
||
fcbstruc.c
|
||
|
||
rx\lockctrl.c
|
||
at rxunlock, we could be called from the cleanup routine on the last
|
||
handle. in such a case, we should just swallow the unlocks. this might be a
|
||
minirdr thing instead.
|
||
|
||
rx\fsctrl.c
|
||
in RxLowIoFsCtlShell, we should have to switch out again. we should have
|
||
handled the peeknamepipe throttling directly in the entry routine.
|
||
|
||
rx\flush.c
|
||
maybe we should try to route for filetypes that we don't understand. or
|
||
at least to have the minirdr signal that we should doit thru some stateful call.
|
||
|
||
rx\fileinfo.c
|
||
i remove the following
|
||
//CODE.IMPROVEMENT having a separate wrapper for every routine is stupid. we should (a) only
|
||
// have it if we need it and (b) preinitialize the buffer and lengthremaining so we don't always
|
||
// have to pass them.
|
||
|
||
[considering this we should fix the trace as well........]
|
||
|
||
and
|
||
//CODE.IMPROVEMENT probably there's a better way than this to figure out whether
|
||
// we need the resource at all or need it shared or need it excl.
|
||
// for name, we don't need it at all since the name never changes
|
||
|
||
and from allinfo handling
|
||
//CODE.IMPROVEMENT.ASHAMED either here or in the minirdr we should glob up call so that multiple trips
|
||
// are not taken
|
||
|
||
rx\fileinfo.c
|
||
you shouldn't do the fsrtloplock check for pipes. this keeps us from not
|
||
allocating the struct
|
||
|
||
rx\fileinfo.c
|
||
should we really change the name in the fcb after a rename. it
|
||
complicates locking a bit for querys
|
||
|
||
rx\dirctrl.c
|
||
no need to allocate templace buffer if it's small enough. most templates
|
||
are probably less than 8.3 in size
|
||
|
||
rx\create.c
|
||
the writeserialization field in the fobx is used by spoolfiles as well
|
||
as pipes. maybe a name change to the specific field could reflect this dual
|
||
usage.
|
||
|
||
rx\create.c
|
||
perhaps we should wait to finalize on a failed open.
|
||
|
||
rx\create.c
|
||
it would be good to "summarize" the fcb names that are logged
|
||
|
||
rx\create.c
|
||
we should have a flag on fcb/netroot somewhere to denote that only
|
||
32-bit operations are possible instead of having every mini have to check.
|
||
|
||
rxce\scavengr.c
|
||
in scavenge-fobxs, starting at the beginning of the list each time is
|
||
bad. what if there is some problem then we'd never get off the first guy.
|
||
|
||
rxce\prefix.c
|
||
i removed this comment from the lookup
|
||
// CODE.IMPROVEMENT we need code to lookup vnetroots MUCH faster....like a 26way cache........
|
||
// failing that, we could at least not do the lookups (and hash computation)
|
||
// until we're on the third component, i.e. \;m:\server\share. the cache is better.
|
||
|
||
rxce\fcbstruc.c
|
||
in finishfcbinitialization, we should not initialize the lock structure
|
||
for pipes. of course, we shouldn't even have a lock structure yet but that's a
|
||
different c.i.
|
||
|
||
rxce\buffring.c
|
||
i removed the following comment from RxChangeBufferingState
|
||
//CODE.IMPROVEMENT we sure use up a lot of log entries in here! maybe we should put this state info
|
||
// on the fcb.
|
||
|
||
BUGBUG
|
||
|
||
rxce\rxcontx.c
|
||
the priority boost is set to disk....probably should be net. actually
|
||
should be mini parameter.
|
||
in the place where we check for advance-only on setfileinfo, we'd need a
|
||
bit different for locals. there are a couple of places.
|
||
|
||
rxce\rxconnct.c
|
||
i removed the following from RxConstructSrvCall
|
||
//BUGBUG we should have some sort of refcount machanism to prevent a minirdr from
|
||
// unregistering while he's part of a calldown
|
||
//also, we shouldn't necessarily be calling down if this guy is not a UNC provider
|
||
|
||
rxce\fcbstruc.c
|
||
in RxWaitForStableCondition, you need to be able to cancel.
|
||
|
||
rxce\fcbstruc.c
|
||
in RxCreateNetRoot,
|
||
//TRUE means case insensitive compares BUGBUG get this from srvcall
|
||
|
||
turns out, the whole issue of casing is incorrectly handled because
|
||
there is no way to get casing handled on a per-call basis in case the
|
||
casesensitive flag is set on open.
|
||
|
||
rxce\fcbstruc.c
|
||
in RxCreateNetFcb,
|
||
//BUGBUG: this should be on the fobx......not on the FCB. it should then be used correctly
|
||
// in the querynameinfo code. same with stripped backslash
|
||
the flag in question is RX_CONTEXT_CREATE_FLAG_ADDEDBACKSLASH.
|
||
|
||
rx\strtstop.c
|
||
in RxStopMinirdr, the comment was removed from the following line
|
||
RxTerminateScavenging(RxContext); //BUGBUG separate into the two parts and make the purge depend on the DObj
|
||
|
||
rx\lockctrl.c
|
||
in RxUnlockOperation, the following comment was removed where we try to
|
||
allocate
|
||
//bugbug we cannot fail........i guess what we'd have to do is to send the unlock
|
||
//from here!
|
||
|
||
rx\fileinfo.c
|
||
in RxQueryStandardInfo, i removed this about the fact that links are
|
||
returned incorrectly
|
||
//BUGBUG there is a problem here with number of links...basically, the deal is that numberoflinks is
|
||
//dynamic whereas everything else in the structure is knowable by the client side under oplock.
|
||
|
||
rx\create.c
|
||
in , i removed this about downlevel canonicalization.
|
||
DOWNLEVEL.BUGBUG we don't really do it yet but we do
|
||
componentize and check for .. and .; for names that come down from win32
|
||
this is fine.....for direct names going to downlevel servers we should do
|
||
more checking maybe.
|
||
|
||
rx\create.c
|
||
in RxCanonicalizeFileNameByServerSpecs, i removed the comment on this
|
||
line both places
|
||
&& RX2C_IS_DOT(Buffer[firstchar+2])) //??BUGBUG what about a alternate stream :.
|
||
|
||
rx\read.c
|
||
need to implement a policy for controlling readahead differently for
|
||
different customers
|
||
|
||
rx\dirctrl.c
|
||
i removed this at the place when the uniarg template is comared
|
||
// then match all names. joejoe....do i really need all this?? ntfs doesn't!
|
||
|
||
rx\create.c
|
||
this from the header comments in RxCreateFromNetRoot:
|
||
joejoe There is some possibility that we should just keep shared on the tablelock until we
|
||
dispatch to the minirdr. That would save one release/acquire per open.
|
||
|
||
rx\cleanup.c
|
||
removed the joejoe from this
|
||
if (NeedDelete || NeedPurge) {
|
||
//joejoe need to put in logic to keep a new read/write from
|
||
//reinitializing
|
||
RxDbgTrace(0, Dbg, ("CleanupPurge:MmFlushImage\n", 0));
|
||
MmFlushImageSection(&capFcb->NonPaged->SectionObjectPointers,
|
||
MmFlushForWrite);
|
||
|
||
rx\cleanup.c
|
||
and from this.....
|
||
KeQuerySystemTime( &CurrentTime );
|
||
|
||
//
|
||
// Note that we HAVE to use BooleanFlagOn() here because
|
||
// FO_FILE_SIZE_CHANGED > 0x80 (i.e., not in the first byte).
|
||
//
|
||
|
||
//joejoe we need to probably do changetime independently
|
||
//perhaps the smartest thing would just to put a basicinfo
|
||
//struc in the fcb
|
||
|
||
rxce\rxconnct.c
|
||
from RxConstructSrvCall, removed the joejoe
|
||
//joejoe we should have a structure tag on this
|
||
pSrvCalldownStructure = RxAllocatePoolWithTag(
|
||
NonPagedPool,
|
||
sizeof(MRX_SRVCALLDOWN_STRUCTURE) +
|
||
(sizeof(MRX_SRVCALL_CALLBACK_CONTEXT) * NumberOfMinirdrs),
|
||
'CSxR' );
|
||
if (pSrvCalldownStructure == NULL) {
|
||
ExReleaseFastMutexUnsafe(&RxData.MinirdrRegistrationMutex);
|
||
return STATUS_INSUFFICIENT_RESOURCES;
|
||
}
|
||
|
||
rxce\fcbstruc.c
|
||
from RxFinalizeSrvOpen, removed the joejoe remark
|
||
if (ThisSrvOpen->Condition == Condition_Good) {
|
||
RxPurgeChangeBufferingStateRequestsForSrvOpen(ThisSrvOpen);
|
||
}
|
||
|
||
//joejoe we could put sme more asserts in here about empty srvfobxlist etc.
|
||
|
||
// close the file.
|
||
MINIRDR_CALL_THROUGH(Status,Fcb->MRxDispatch,MRxForceClosed,((PMRX_SRV_OPEN)ThisSrvOpen));
|
||
}
|
||
|
||
rxce\buffring.c
|
||
from RxChangeBufferingState, removed joejoe
|
||
MINIRDR_CALL_THROUGH(
|
||
Status,
|
||
Fcb->MRxDispatch,
|
||
MRxCompleteBufferingStateChangeRequest,
|
||
(RxContext,(PMRX_SRV_OPEN)SrvOpen,Context));
|
||
}
|
||
|
||
//joejoe this could fail!!!! we should place the fcb into a
|
||
// "failed oplock break" state.
|
||
|
||
RxDereferenceAndDeleteRxContext(RxContext);
|
||
}
|
||
|
||
AND
|
||
|
||
} finally {
|
||
ClearFlag(Fcb->FcbState,FCB_STATE_BUFFERSTATE_CHANGING); //this is informational for error recovery
|
||
//joejoe there should be a callout here for a readpolicy...not vanilla..........
|
||
}
|
||
|
||
|