Skip to content

Commit

Permalink
Improve error handling for writing (ref #9)
Browse files Browse the repository at this point in the history
* Replace writeable entries with -[ZZMutableArchive updateEntries:error:] method.

* Stream, data and data consumer blocks now should report errors.

* Revise error codes to be consistent with write errors.

* Internal API now report file manager, POSIX and stream errors.

* Unit tests for creating or inserting erroneous entries.
  • Loading branch information
pixelglow committed Feb 2, 2013
1 parent b257d81 commit 74daa74
Show file tree
Hide file tree
Showing 29 changed files with 911 additions and 372 deletions.
15 changes: 10 additions & 5 deletions zipzap/ZZArchive.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
/**
* The array of <ZZArchiveEntry> entries within this archive.
*/
@property (readonly, copy, nonatomic) NSArray* entries;
@property (readonly, nonatomic) NSArray* entries;

/**
* Creates a new archive with the zip file at the given file URL.
Expand Down Expand Up @@ -111,10 +111,15 @@
@interface ZZMutableArchive : ZZArchive

/**
* The array of <ZZArchiveEntry> entries within this archive.
* To write new entries in the zip file, set this property to a different array of <ZZArchiveEntry> entries.
* When you set this property, any old entries should be considered invalid.
* Updates the entries and writes them to the source.
*
* @param newEntries The entries to update to, may contain some or all existing entries.
* @param error The error information when an error occurs. Pass in *nil* if you do not want error information.
* @return Whether the update was successful or not.
*
* @remarks If the write fails and the entries are completely new, the existing zip file will be untouched. Instead, if the write fails and the entries contain some or all existing entries, the zip file may be corrupted. In this case, the error information will report the ZZReplaceWriteErrorCode error code.
*/
@property (copy, nonatomic) NSArray* entries;
- (BOOL)updateEntries:(NSArray*)newEntries
error:(NSError**)error;

@end
149 changes: 94 additions & 55 deletions zipzap/ZZArchive.mm
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ - (BOOL)load:(NSError**)error
NSError* __autoreleasing readError;
NSData* contents = [_channel openInput:&readError];
if (!contents)
return ZZRaiseError(error, ZZReadErrorCode, @{NSUnderlyingErrorKey : readError});
return ZZRaiseError(error, ZZOpenReadErrorCode, @{NSUnderlyingErrorKey : readError});

This comment has been minimized.

Copy link
@mikeabdullah

mikeabdullah Feb 2, 2013

Contributor

Is there any advantage to reporting your own error code here? Cocoa provides perfectly decent errors itself

This comment has been minimized.

Copy link
@pixelglow

pixelglow Feb 2, 2013

Author Owner

Having your own errors is useful to report what stage the read is in, or even the failed index in particular cases. In any case the underlying Cocoa or POSIX error is also reported.


// search for the end of directory signature in last 64K of file
const uint8_t* beginContent = (const uint8_t*)contents.bytes;
Expand Down Expand Up @@ -125,7 +125,7 @@ - (BOOL)load:(NSError**)error
// end of central directory occurs at actual end of the zip
|| endContent
!= endOfCentralDirectory + sizeof(ZZEndOfCentralDirectory) + endOfCentralDirectoryRecord->zipFileCommentLength)
return ZZRaiseError(error, ZZBadEndOfCentralDirectoryErrorCode, nil);
return ZZRaiseError(error, ZZEndOfCentralDirectoryReadErrorCode, nil);

// add an entry for each central header in the sequence
ZZCentralFileHeader* nextCentralFileHeader = (ZZCentralFileHeader*)(beginContent
Expand All @@ -142,7 +142,7 @@ - (BOOL)load:(NSError**)error
// local file occurs before first central file header, and has enough minimal space for at least local file
|| nextCentralFileHeader->relativeOffsetOfLocalHeader + sizeof(ZZLocalFileHeader)
> endOfCentralDirectoryRecord->offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber)
return ZZRaiseError(error, ZZBadCentralFileErrorCode, @{ZZEntryIndexKey : @(index)});
return ZZRaiseError(error, ZZCentralFileHeaderReadErrorCode, @{ZZEntryIndexKey : @(index)});

ZZLocalFileHeader* nextLocalFileHeader = (ZZLocalFileHeader*)(beginContent
+ nextCentralFileHeader->relativeOffsetOfLocalHeader);
Expand All @@ -164,7 +164,8 @@ - (BOOL)load:(NSError**)error

@implementation ZZMutableArchive

- (void)setEntries:(NSArray*)newEntries
- (BOOL)updateEntries:(NSArray*)newEntries
error:(NSError**)error
{
// NOTE: we want to avoid loading at all when entries are being overwritten, even in the face of lazy loading:
// consider that nil _contents implies that no valid entries have been loaded, and newEntries cannot possibly contain any of our old entries
Expand All @@ -182,10 +183,9 @@ - (void)setEntries:(NSArray*)newEntries
// get an entry writer for each new entry
NSMutableArray* newEntryWriters = [NSMutableArray array];

[newEntries enumerateObjectsUsingBlock:^(ZZArchiveEntry *anEntry, NSUInteger idx, BOOL *stop)
[newEntries enumerateObjectsUsingBlock:^(ZZArchiveEntry *anEntry, NSUInteger index, BOOL* stop)
{

[newEntryWriters addObject:[anEntry writerCanSkipLocalFile:(idx < skipIndex)]];
[newEntryWriters addObject:[anEntry writerCanSkipLocalFile:index < skipIndex]];
}];

// clear entries + content
Expand All @@ -195,59 +195,98 @@ - (void)setEntries:(NSArray*)newEntries
// skip the initial matching entries
uint32_t initialSkip = skipIndex > 0 ? [[newEntryWriters objectAtIndex:skipIndex - 1] offsetToLocalFileEnd] : 0;

NSError* __autoreleasing underlyingError;

// create a temp channel for all output
id<ZZChannel> temporaryChannel = [_channel temporaryChannel];
id<ZZChannelOutput> temporaryChannelOutput = [temporaryChannel openOutputWithOffsetBias:initialSkip];


// write out local files, recording which are valid
NSMutableIndexSet* goodEntries = [NSMutableIndexSet indexSetWithIndexesInRange:NSMakeRange(0, newEntries.count)];

[newEntryWriters enumerateObjectsUsingBlock:^(id <ZZArchiveEntryWriter> entryWriter, NSUInteger idx, BOOL *stop)
{
if (idx >= skipIndex && ![entryWriter writeLocalFileToChannelOutput:temporaryChannelOutput])
[goodEntries removeIndex:idx];
}];

ZZEndOfCentralDirectory endOfCentralDirectory;
endOfCentralDirectory.signature = ZZEndOfCentralDirectory::sign;
endOfCentralDirectory.numberOfThisDisk
= endOfCentralDirectory.numberOfTheDiskWithTheStartOfTheCentralDirectory
= 0;
endOfCentralDirectory.totalNumberOfEntriesInTheCentralDirectoryOnThisDisk
= endOfCentralDirectory.totalNumberOfEntriesInTheCentralDirectory
= goodEntries.count;
endOfCentralDirectory.offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber = temporaryChannelOutput.offset;

// write out central file headers
[newEntryWriters enumerateObjectsAtIndexes:goodEntries options:0 usingBlock:^(id <ZZArchiveEntryWriter> anEntryWriter, NSUInteger idx, BOOL *stop)
{
[anEntryWriter writeCentralFileHeaderToChannelOutput:temporaryChannelOutput];
}];

endOfCentralDirectory.sizeOfTheCentralDirectory = temporaryChannelOutput.offset
- endOfCentralDirectory.offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber;
endOfCentralDirectory.zipFileCommentLength = 0;
id<ZZChannel> temporaryChannel = [_channel temporaryChannel:&underlyingError];
if (!temporaryChannel)
return ZZRaiseError(error, ZZOpenWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});

// write out the end of central directory
[temporaryChannelOutput write:[NSData dataWithBytesNoCopy:&endOfCentralDirectory
length:sizeof(endOfCentralDirectory)
freeWhenDone:NO]];
[temporaryChannelOutput close];

if (initialSkip)
@try
{
// something skipped, append the temporary channel contents at the skipped offset
id<ZZChannelOutput> channelOutput = [_channel openOutputWithOffsetBias:0];
channelOutput.offset = initialSkip;
[channelOutput write:[temporaryChannel openInput:nil]];
[channelOutput close];
// open the channel
id<ZZChannelOutput> temporaryChannelOutput = [temporaryChannel openOutputWithOffsetBias:initialSkip
error:&underlyingError];
if (!temporaryChannelOutput)
return ZZRaiseError(error, ZZOpenWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});

@try
{
// write out local files
for (NSUInteger index = skipIndex; index < newEntriesCount; ++index)
if (![[newEntryWriters objectAtIndex:index] writeLocalFileToChannelOutput:temporaryChannelOutput
error:&underlyingError])
return ZZRaiseError(error, ZZLocalFileWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError, ZZEntryIndexKey : @(index)});

ZZEndOfCentralDirectory endOfCentralDirectory;
endOfCentralDirectory.signature = ZZEndOfCentralDirectory::sign;
endOfCentralDirectory.numberOfThisDisk
= endOfCentralDirectory.numberOfTheDiskWithTheStartOfTheCentralDirectory
= 0;
endOfCentralDirectory.totalNumberOfEntriesInTheCentralDirectoryOnThisDisk
= endOfCentralDirectory.totalNumberOfEntriesInTheCentralDirectory
= newEntriesCount;
endOfCentralDirectory.offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber = [temporaryChannelOutput offset];

// write out central file headers
for (NSUInteger index = 0; index < newEntriesCount; ++index)
if (![[newEntryWriters objectAtIndex:index] writeCentralFileHeaderToChannelOutput:temporaryChannelOutput
error:&underlyingError])
return ZZRaiseError(error, ZZCentralFileHeaderWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError, ZZEntryIndexKey : @(index)});

endOfCentralDirectory.sizeOfTheCentralDirectory = [temporaryChannelOutput offset]
- endOfCentralDirectory.offsetOfStartOfCentralDirectoryWithRespectToTheStartingDiskNumber;
endOfCentralDirectory.zipFileCommentLength = 0;

// write out the end of central directory
if (![temporaryChannelOutput writeData:[NSData dataWithBytesNoCopy:&endOfCentralDirectory
length:sizeof(endOfCentralDirectory)
freeWhenDone:NO]
error:&underlyingError])
return ZZRaiseError(error, ZZEndOfCentralDirectoryWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});
}
@finally
{
[temporaryChannelOutput close];
}

if (initialSkip)
{
// something skipped, append the temporary channel contents at the skipped offset
id<ZZChannelOutput> channelOutput = [_channel openOutputWithOffsetBias:0
error:&underlyingError];
if (!channelOutput)
return ZZRaiseError(error, ZZReplaceWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});

@try
{
NSData* channelInput = [temporaryChannel openInput:&underlyingError];
if (!channelInput
|| ![channelOutput seekToOffset:initialSkip
error:&underlyingError]
|| ![channelOutput writeData:channelInput
error:&underlyingError]
|| ![channelOutput truncateAtOffset:[channelOutput offset]
error:&underlyingError])
return ZZRaiseError(error, ZZReplaceWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});
}
@finally
{
[channelOutput close];
}
}
else
// nothing skipped, temporary channel is entire contents: simply replace the original
if (![_channel replaceWithChannel:temporaryChannel
error:&underlyingError])
return ZZRaiseError(error, ZZReplaceWriteErrorCode, @{NSUnderlyingErrorKey : underlyingError});
}
@finally
{
[_channel removeTemporaries];
}
else
// nothing skipped, temporary channel is entire contents: simply replace the original
[_channel replaceWithChannel:temporaryChannel];

[_channel removeTemporaries];
return YES;
}

@end
12 changes: 6 additions & 6 deletions zipzap/ZZArchiveEntry.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@
*/
+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
streamBlock:(BOOL(^)(NSOutputStream* stream))streamBlock;
streamBlock:(BOOL(^)(NSOutputStream* stream, NSError** error))streamBlock;

/**
* Creates a new file entry from a data callback.
Expand All @@ -102,7 +102,7 @@
*/
+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
dataBlock:(NSData*(^)())dataBlock;
dataBlock:(NSData*(^)(NSError** error))dataBlock;

/**
* Creates a new file entry from a data-consuming callback.
Expand All @@ -114,7 +114,7 @@
*/
+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer))dataConsumerBlock;
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer, NSError** error))dataConsumerBlock;

/**
* Creates a new directory entry.
Expand Down Expand Up @@ -142,9 +142,9 @@
fileMode:(mode_t)fileMode
lastModified:(NSDate*)lastModified
compressionLevel:(NSInteger)compressionLevel
dataBlock:(NSData*(^)())dataBlock
streamBlock:(BOOL(^)(NSOutputStream* stream))streamBlock
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer))dataConsumerBlock;
dataBlock:(NSData*(^)(NSError** error))dataBlock
streamBlock:(BOOL(^)(NSOutputStream* stream, NSError** error))streamBlock
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer, NSError** error))dataConsumerBlock;

/**
* Checks whether the entry file is consistent.
Expand Down
12 changes: 6 additions & 6 deletions zipzap/ZZArchiveEntry.m
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ @implementation ZZArchiveEntry

+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
dataBlock:(NSData*(^)())dataBlock
dataBlock:(NSData*(^)(NSError** error))dataBlock
{
return [self archiveEntryWithFileName:fileName
fileMode:S_IFREG | S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
Expand All @@ -28,7 +28,7 @@ + (instancetype)archiveEntryWithFileName:(NSString*)fileName

+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
streamBlock:(BOOL(^)(NSOutputStream* stream))streamBlock
streamBlock:(BOOL(^)(NSOutputStream* stream, NSError** error))streamBlock
{
return [self archiveEntryWithFileName:fileName
fileMode:S_IFREG | S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
Expand All @@ -41,7 +41,7 @@ + (instancetype)archiveEntryWithFileName:(NSString*)fileName

+ (instancetype)archiveEntryWithFileName:(NSString*)fileName
compress:(BOOL)compress
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer))dataConsumerBlock
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer, NSError** error))dataConsumerBlock
{
return [self archiveEntryWithFileName:fileName
fileMode:S_IFREG | S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH
Expand All @@ -67,9 +67,9 @@ + (instancetype)archiveEntryWithFileName:(NSString*)fileName
fileMode:(mode_t)fileMode
lastModified:(NSDate*)lastModified
compressionLevel:(NSInteger)compressionLevel
dataBlock:(NSData*(^)())dataBlock
streamBlock:(BOOL(^)(NSOutputStream* stream))streamBlock
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer))dataConsumerBlock
dataBlock:(NSData*(^)(NSError** error))dataBlock
streamBlock:(BOOL(^)(NSOutputStream* stream, NSError** error))streamBlock
dataConsumerBlock:(BOOL(^)(CGDataConsumerRef dataConsumer, NSError** error))dataConsumerBlock
{
return [[ZZNewArchiveEntry alloc] initWithFileName:fileName
fileMode:fileMode
Expand Down
7 changes: 4 additions & 3 deletions zipzap/ZZArchiveEntryWriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
@protocol ZZArchiveEntryWriter

- (uint32_t)offsetToLocalFileEnd;
- (BOOL)writeLocalFileToChannelOutput:(id<ZZChannelOutput>)channelOutput;
- (void)writeCentralFileHeaderToChannelOutput:(id<ZZChannelOutput>)channelOutput;

- (BOOL)writeLocalFileToChannelOutput:(id<ZZChannelOutput>)channelOutput
error:(NSError**)error;
- (BOOL)writeCentralFileHeaderToChannelOutput:(id<ZZChannelOutput>)channelOutput
error:(NSError**)error;
@end
8 changes: 5 additions & 3 deletions zipzap/ZZChannel.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@

@property (readonly, nonatomic) NSURL* URL;

- (instancetype)temporaryChannel;
- (BOOL)replaceWithChannel:(id<ZZChannel>)channel;
- (instancetype)temporaryChannel:(NSError**)error;
- (BOOL)replaceWithChannel:(id<ZZChannel>)channel
error:(NSError**)error;
- (void)removeTemporaries;

- (NSData*)openInput:(NSError**)error;
- (id<ZZChannelOutput>)openOutputWithOffsetBias:(uint32_t)offsetBias;
- (id<ZZChannelOutput>)openOutputWithOffsetBias:(uint32_t)offsetBias
error:(NSError**)error;

@end
9 changes: 7 additions & 2 deletions zipzap/ZZChannelOutput.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@

@protocol ZZChannelOutput

@property (nonatomic) uint32_t offset;
- (uint32_t)offset;
- (BOOL)seekToOffset:(uint32_t)offset
error:(NSError**)error;

- (void)write:(NSData*)data;
- (BOOL)writeData:(NSData*)data
error:(NSError**)error;
- (BOOL)truncateAtOffset:(uint32_t)offset
error:(NSError**)error;
- (void)close;

@end
8 changes: 5 additions & 3 deletions zipzap/ZZDataChannel.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

- (id)initWithData:(NSData*)data;

- (instancetype)temporaryChannel;
- (BOOL)replaceWithChannel:(id<ZZChannel>)channel;
- (instancetype)temporaryChannel:(NSError**)error;
- (BOOL)replaceWithChannel:(id<ZZChannel>)channel
error:(NSError**)error;
- (void)removeTemporaries;

- (NSData*)openInput:(NSError**)error;
- (id<ZZChannelOutput>)openOutputWithOffsetBias:(uint32_t)offsetBias;
- (id<ZZChannelOutput>)openOutputWithOffsetBias:(uint32_t)offsetBias
error:(NSError**)error;

@end
4 changes: 3 additions & 1 deletion zipzap/ZZDataChannel.m
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,13 @@ - (NSURL*)URL
return nil;
}

- (instancetype)temporaryChannel
- (instancetype)temporaryChannel:(NSError**)error
{
return [[ZZDataChannel alloc] initWithData:[NSMutableData data]];
}

- (BOOL)replaceWithChannel:(id<ZZChannel>)channel
error:(NSError**)error
{
[(NSMutableData*)_allData setData:((ZZDataChannel*)channel)->_allData];
return YES;
Expand All @@ -47,6 +48,7 @@ - (NSData*)openInput:(NSError**)error
}

- (id<ZZChannelOutput>)openOutputWithOffsetBias:(uint32_t)offsetBias
error:(NSError**)error
{
return [[ZZDataChannelOutput alloc] initWithData:(NSMutableData*)_allData
offsetBias:offsetBias];
Expand Down
Loading

0 comments on commit 74daa74

Please sign in to comment.