Cleaned up archive_read_disk_descend functions #1606
Conversation
| "archive_read_disk_descend"); | ||
|
|
||
| if (t->visit_type != TREE_REGULAR || !t->descend) | ||
| if (!archive_read_disk_can_descend(_a)) |
There was a problem hiding this comment.
I think there is a problem in the archive_read_disk_can_descend() function. It calls archive_check_magic() which can return ARCHIVE_FATAL which becomes a return value of archive_read_disk_can_descend(). As it converted to bool that will cause the rest of the function to be skipped in case of bad magic. If the check above is removed as redundant in the future - this can lead to behavior change. Ideally, archive_read_disk_can_descend() should return ARCHIVE_OK or other statuses instead of bool and checked appropriately at callers. There are several call sites where there is no archive_check_magic() before invocation of archive_read_disk_can_descend().
There was a problem hiding this comment.
archive_check_magic is a sanity check. If it triggers, the caller is doing something completely wrong.
There was a problem hiding this comment.
The problem is bad interface - the archive_read_disk_can_descend() return BOOL and it is true when the sanity check fails.
There was a problem hiding this comment.
Again, irrelevant. It's checked already just above this line.
|
For a trivial function like this, it might make sense to remove the magic check. Again, that check is there is to help developers use libarchive by giving them a clear error when they call functions that aren't reasonable in a certain context. For a trivial status function like this, that check is less useful. |
Moved the code changes out of the documentation changes to the man pages for archive_read_disk.