-
-
Notifications
You must be signed in to change notification settings - Fork 36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Setters for ctime, mtime and atime. #50
Conversation
Please keep the Git history clean. git remote add upstream "https://github.com/Changaco/python-libarchive-c.git"
git fetch upstream
git rebase -i upstream/master
git push -f |
added atime, mtime and ctime properties
Sure thing! Is it OK now? |
@@ -127,9 +127,12 @@ def ffi(name, argtypes, restype, errcheck=None): | |||
ffi('entry_rdevmajor', [c_archive_entry_p], c_uint) | |||
ffi('entry_rdevminor', [c_archive_entry_p], c_uint) | |||
|
|||
ffi('entry_set_size', [c_archive_entry_p, c_longlong], c_int) | |||
ffi('entry_set_size', [c_archive_entry_p, c_longlong], c_longlong) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this change is necessary to allow compressing big files (our files are huge).
BTW longlong is the type that actually matches the C signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't match the signature. The archive_entry_set_*
functions don't actually return anything (void
), so the last argument to ffi
should probably be None
. I'm not sure why it's currently c_int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought about this commit f301a92
yes, it's completely unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Changaco should I change it back to c_int
or to None
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed them to None
. The tests passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I was at it I changed the argument type of entry_set_filetype
from c_int
to c_uint
since the C signature says unsigned int
.
@@ -240,5 +243,7 @@ def ffi(name, argtypes, restype, errcheck=None): | |||
c_int, check_int) | |||
ffi('write_finish_entry', [c_archive_p], c_int, check_int) | |||
|
|||
ffi('write_fail', [c_archive_p], c_int, check_int) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function doesn't seem to be used, why do you want to add it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not used directly by any of context managers in python-libarchive-c, but we use it in our project to allow cancelling creation of archive. Since this library is a convenient wrapper around libarchive we think that it's the best place for it (and make this wrapper more complete).
Turns out that if you want to discard the archive you were creating, you can not just stop creating it. Context managers will close the archive, and libarchive will try to fill the missing contents with 000000000000000000..., and that can cause memory overflows. You have to mark the archive as failed (by that function) to tell libarchive that it should give up any work on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then shouldn't the context managers call write_fail
when an exception is raised?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a separate issue for that: #51.
Hey!
I have added setters for ctime, mtime and atime. Hope it might be useful!