GNOME Bugzilla – Bug 775253
Ask whether to unpack when archive could file up disk
Last modified: 2017-03-29 15:57:52 UTC
I'm not sure what the heuristics should be, but we should probably avoid unpacking huge archives on double-click, or middling-size archives if disk space is low. For example, unpacking a 2 or 4 GB .tar.gz that contains a game when I only have slightly above that amount in disk space. This would also avoid having the housekeeping plugin popping up a notification during unpacking.
GVfs provides archive backend (which is a little bit forgotten), which is based on libarchive, same as the built-in solution in Nautilus (so it has same features). The backend is quite fast because it reads just headers to show the files. So, the archive content might be shown using the backend on double-click (it doesn't need any additional space) instead of extracting and button might be shown on the top in order to allow extracting the archive... What do you think?
(In reply to Ondrej Holy from comment #1) > GVfs provides archive backend (which is a little bit forgotten), which is > based on libarchive, same as the built-in solution in Nautilus (so it has > same features). The backend is quite fast because it reads just headers to > show the files. So, the archive content might be shown using the backend on > double-click (it doesn't need any additional space) instead of extracting > and button might be shown on the top in order to allow extracting the > archive... What do you think? This isn't about changing the current interface, but rather about avoiding the worst case scenarios.
(In reply to Ondrej Holy from comment #1) > GVfs provides archive backend (which is a little bit forgotten), which is > based on libarchive, same as the built-in solution in Nautilus (so it has > same features). The backend is quite fast because it reads just headers to > show the files. So, the archive content might be shown using the backend on > double-click (it doesn't need any additional space) instead of extracting > and button might be shown on the top in order to allow extracting the > archive... What do you think? That's the opposite of what we want I think, that's the main reason why gnome-autoar exists in the first place. But it's something I pondered (I think we discussed between us too right?) and worth asking designers. But yeah this bug report is not about that, but rather avoid when something bad is going to happen.
Created attachment 347855 [details] [review] extraction: check filespace before extraction Sometimes, the extractions starts without enough filesystem space to store the extracted files. This commit checks for filespace before the extraction takes place.
Review of attachment 347855 [details] [review]: Hey! good work, the code looks nice. However it cannot ever work I think, did you try it? Also some nitpicks ::: src/nautilus-file-operations.c @@ +8401,3 @@ + basename = get_basename (source_file); + + g_autofree gchar *basename = NULL; Here you are unconditionally setting the error. Is that what you wanted? @@ +8412,3 @@ + + if (total_size > g_file_info_get_attribute_uint64 (fsinfo, + source_file = autoar_extractor_get_source_file (extractor); can you put this in a variable? @@ +8414,3 @@ + G_FILE_ATTRIBUTE_FILESYSTEM_FREE) && total_size != -1) + { + basename = get_basename (source_file); UI has to be ran in the main thread, this should actually not work. Did you test the patch? @@ +8424,3 @@ + if (response_id == 0 || response_id == GTK_RESPONSE_DELETE_EVENT) + { + 4 spaces
Created attachment 348548 [details] Forced error. It works for me when I force an error. run_error -> run_simple_dialog_va -> g_main_context_invoke which runs the dialog in the UI thread. updating the changes :)
I would suggest changing the error string to something more natural sounding, for example: "Not enough free space to extract “%s”."
Created attachment 348577 [details] [review] extraction: check filespace before extraction Sometimes, the extractions starts without enough filesystem space to store the extracted files. This commit checks for filespace before the extraction takes place.
Review of attachment 348577 [details] [review]: The commit message tittle is missing the correct file that you modified. The commit message is also missing some context, also it says "sometimes" but not sure in what cases, you can explain that context a little more. ::: src/nautilus-file-operations.c @@ +8410,3 @@ + G_FILE_ATTRIBUTE_FILESYSTEM_FREE); + + extract_job = user_data; does the second condition makes sense at all? when can -1 be bigger than free_size? Also, when total_size can be -1? I can see in autoar: /* If we are unable to determine the total size, set it to a positive * number to prevent strange percentage. */ if (self->total_size <= 0) self->total_size = G_MAXUINT64; Which is IMHO wrong anyway. We should have an API for querying whether the size can be determined or not. For the time being anyway, but can just assume the number is positive. @@ +8423,3 @@ + NULL); + + G_FILE_ATTRIBUTE_FILESYSTEM_FREE "," what is 0? Use constants like you do in the other one. Also, does it make sense to have this conditional at all? In what case we wouldn't abort the job?
(In reply to Carlos Soriano from comment #9) > Review of attachment 348577 [details] [review] [review]: > > The commit message tittle is missing the correct file that you modified. > The commit message is also missing some context, also it says "sometimes" > but not sure in what cases, you can explain that context a little more. > > ::: src/nautilus-file-operations.c > @@ +8410,3 @@ > + > G_FILE_ATTRIBUTE_FILESYSTEM_FREE); > + > + extract_job = user_data; > > does the second condition makes sense at all? when can -1 be bigger than > free_size? > > Also, when total_size can be -1? > I can see in autoar: > /* If we are unable to determine the total size, set it to a positive > * number to prevent strange percentage. */ > if (self->total_size <= 0) > self->total_size = G_MAXUINT64; > > Which is IMHO wrong anyway. We should have an API for querying whether the > size can be determined or not. For the time being anyway, but can just > assume the number is positive. > total_size is an uint64. So, int -1 = 18,446,744,073,709,551,615 uint64 it will for most cases be more than the available space. Hence my usage of the -1. > @@ +8423,3 @@ > + NULL); > + > + G_FILE_ATTRIBUTE_FILESYSTEM_FREE > "," > > what is 0? Use constants like you do in the other one. Also, does it make > sense to have this conditional at all? In what case we wouldn't abort the > job? Removing the conditional statement. :)
Created attachment 348850 [details] [review] file-operations: check filespace before extracting Currently, the extraction starts without checking for enough filesystem space to store the extracted files. This commit checks for filespace before the extraction takes place.
> > total_size is an uint64. So, int -1 = 18,446,744,073,709,551,615 uint64 > > it will for most cases be more than the available space. > > Hence my usage of the -1. > ugh, that's quite bad... also I'm not sure this is really well defined in the standard (looking http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf ) so it could even be implementation dependent.
Review of attachment 348850 [details] [review]: The commit message looks a little small for such a change, and a comment explaining the G_MAXUINT64 would be needed since nowhere in that code is obvious or explained why this value, but I can add that. Otherwise looks good to me!
(In reply to Carlos Soriano from comment #12) > > > > total_size is an uint64. So, int -1 = 18,446,744,073,709,551,615 uint64 > > > > it will for most cases be more than the available space. > > > > Hence my usage of the -1. > > > ugh, that's quite bad... also I'm not sure this is really well defined in > the standard (looking > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf ) so it could even > be implementation dependent. No, it’s well-defined. The signed operand of lower or equal rank is converted to the unsigned type of the greater rank, but I’m fairly sure that compilers complain about such implicit conversions with the right flags. There are other ways to get the maximum value.
Review of attachment 348850 [details] [review]: ::: src/nautilus-file-operations.c @@ +8404,3 @@ + G_FILE_ATTRIBUTE_FILESYSTEM_FREE "," + G_FILE_ATTRIBUTE_FILESYSTEM_READONLY, + guint64 free_size; This is wrong too, you should see a warning on the compiler, you are passing a pointer to a pointer, this will make it crash if something tries to modify it.
(In reply to Ernestas Kulik from comment #14) > No, it’s well-defined. The signed operand of lower or equal rank is > converted to the unsigned type of the greater rank, but I’m fairly sure that > compilers complain about such implicit conversions with the right flags. > There are other ways to get the maximum value. Yeah, it's fine: http://stackoverflow.com/questions/7221409/is-unsigned-integer-subtraction-defined-behavior But that doesn't make it a good idea. This is what G_MAXUINT64 is for. Or UINT64_MAX from stdint.h....
Attachment 348850 [details] pushed as f30a589 - file-operations: check filespace before extracting
(In reply to Ernestas Kulik from comment #14) > (In reply to Carlos Soriano from comment #12) > > > > > > total_size is an uint64. So, int -1 = 18,446,744,073,709,551,615 uint64 > > > > > > it will for most cases be more than the available space. > > > > > > Hence my usage of the -1. > > > > > ugh, that's quite bad... also I'm not sure this is really well defined in > > the standard (looking > > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1124.pdf ) so it could even > > be implementation dependent. > > No, it’s well-defined. The signed operand of lower or equal rank is > converted to the unsigned type of the greater rank, but I’m fairly sure that > compilers complain about such implicit conversions with the right flags. > There are other ways to get the maximum value. I was more talking about -1 being exactly G_MAXUINT64 (like the link Michael pointed to), it's not clear to me that from that sentence in the standard means how it converts it, and that can be implementation dependent no? (or I do not understand completely that sentence of the standard).
(In reply to Carlos Soriano from comment #18) > I was more talking about -1 being exactly G_MAXUINT64 (like the link Michael > pointed to) I know. > it's not clear to me that from that sentence in the standard > means how it converts it, and that can be implementation dependent no? (or I > do not understand completely that sentence of the standard). I quoted the wrong part, sorry. ISO/IEC 9899:1999 §6.3.1.3: > [I]f the new type is unsigned, the value is converted by repeatedly adding or > subtracting one more than the maximum value that can be represented in the new > type until the value is in the range of the new type. Again, well-defined.
Ah yeah, read that, but misunderstood it.
Never knew there was a 500 page document on C standard. I will try to take it into account from next patch