PI Vol3, FvCheck pseudocode bugs

[valerij zaporogeci <vlrzprgts@gmail.com>]

Hi everyone. Reading the PI specification Volume 3 on FV stuff, I
faced a lot of problems
with figuring out what the pseudocode for Fv check should mean. Of
course it's probably my fault, but still, the pseudocode has some
obvious errata. Here I collected some:

1. Function FileCheck() states it "returns TRUE if the file is OK"
But in the function FvCheck() we see:

if(FileCheck(Fv, FilePtr) != 0){
	FAILURE(); // inconsistent file system
}

2. FileCheck() on the set flag EFI_FILE_CONSTRUCTION sets
EFI_FILE_HEADER_INVALID and returns TRUE. The question is how next
operations on that FV are going to locate the _next_ file header
behind that with the HEADER_INVALID bit set?

3. FileCheck() on the set flag MARKED_FOR_UPDATE does check for
DATA_VALID flag and if it is not set, returns FALSE. But the
specification says a higher bit _overwrites_ meaning of lower ones.
Why this check and what for? Could a file MARKED_FOR_UPDATE
have DATA_VALID flag NOT set?

4. FileCheck() on the MARKED_FOR_UPDATE flag set for the case where
the MARKED file _doesn't_ have a DATA_VALID duplicate file, whether
clears MARKED bit directly or by copying the file. First question -
what happens if there is no place to copy the file? And the most
important - this situation of MARKED file and a non DATA_VALID
duplicate happens on update failure, where the NEW file had no chance
to get to the state DATA_VALID. This pseudocode suggests just to roll
back the marking by means whether of copying or just clearing the bit.
And then returns TRUE, meaning everything is fine. But is it? Such a
check will result in half files being updated and half NOT, without
any warning. It's hard to call it a
"consistent" state. Maybe, here the function should signal that there
is a need to continue an update from this stopped point of updating
the MARKED file. NOT just rolling back the MARKED flag?

5. FileCheck() on the DELETED flag verifies both header checksum and
file data checksum of the file. But the specification says that
deleted file header is only valid and only "in as much as its length
field is used in locating the next file in the firmware volume." So
why checking the file data checksum?

6. Traversing the FV, FvCheck() relies on Exists() for stopping the traversal:
for(FilePtr = FirstFile; Exists(Fv, FilePtr); FilePtr = NextFile(Fv, FilePtr)){
	if(FileCheck (Fv, FilePtr) != 0){
		FAILURE(); // inconsistent file system
	}
}

Exists() in turn relies on BufferErased():

BOOLEAN Exists(Fv, FilePtr){
	return (BufferErased (Fv.ErasePolarity, FilePtr, sizeof
(EFI_FIRMWARE_VOLUME_HEADER) == FALSE);
}

By the way, here we have parenthesis mismatch (copied above as is)
Probably this was meant:

BOOLEAN Exists(Fv, FilePtr){
	return ( BufferErased(Fv.ErasePolarity, FilePtr,
sizeof(EFI_FIRMWARE_VOLUME_HEADER)) == FALSE );
}

As it is seen, BufferErased() is supposed to check whether the buffer
has something written in it, or not. First, the check for every file
is done with sizeof(EFI_FIRMWARE_VOLUME_HEADER) size parameter,
which is not right I believe, EFI_FFS_FILE_HEADER or
EFI_FFS_FILE_HEADER2 should be used instead. And the second, this
checking for the end of traversal implies that in the end of the FV
lies the area with all zeros or ones, depending on ERASE_POLARITY,
thus a not programmed, free space. But why it should? And if it is not
the case, this check is going to cause a constant check failures or
even worse - different aborts, while FV is OK.
I thought, for the purpose of finding the end of file traversal, there
is concept of VTF.
Isn't it for that? Wouldn't it be safer here to check for VTF file? Like:

FilePtr = FirstFile;
while(TRUE){
	if(FileCheck (Fv, FilePtr) != 0){
		FAILURE(); // inconsistent file system
	}
	if(IsFileVtf(FilePtr))break;
	FilePtr = NextFile(Fv, FilePtr);
}
And also, considering the property of Vtf of having its last byte as
the last byte of FV, how
this file could be updated at all? With this mechanism of marking for
update and creating a duplicate?

7. There is no pseudocode for CheckFreeSpace() function. Nor
description what it means and does. What a free space if we have VTF?

Thank you.