Skip to content
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

Added check for files bigger than 4GB #186

Merged
merged 5 commits into from
May 2, 2018
Merged

Conversation

WaxyMocha
Copy link
Contributor

If source directory contain file with size grater than 4GB, script will end with message:

Error: File is larger than that supported by the fat32 filesystem. Use NTFS (--target-filesystem NTFS).

Copy link
Collaborator

@brlin-tw brlin-tw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for contributing, however, some aspects of the patch can still be improved. Feel free to ask any questions on meeting the merge criteria.

src/woeusb Outdated
@@ -272,6 +272,11 @@ init(){
exit 1
fi

if [ "${target_filesystem_type}" == 'FAT' ]; then
check_for_big_files\
"${source_fs_mountpoint}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function arguments in line-continuation requires an additional indentation(TAB)

src/woeusb Outdated
local source_fs_mountpoint="${1}"

for i in $(find ${source_fs_mountpoint}/ -type f); do
if (( $(du $i | cut -f -1) > 4194303 )); then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • stat(1) should be more suitable than du(1) to determine file size
  • Why 4194303? Document the unit conversion in the above comments

src/woeusb Outdated
check_for_big_files(){
local source_fs_mountpoint="${1}"

for i in $(find ${source_fs_mountpoint}/ -type f); do
Copy link
Collaborator

@brlin-tw brlin-tw May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • "${quote}" "$(all)" "${the}" "$(expansions)" (when possible)
  • The trailing slash after source_fs_mountpoint is unnecessary
  • The loop will fail when the source_fs contains filepaths with spaces, checkout delimiter - Reading null delimited strings through a Bash loop - Stack Overflow for a robust solution using process substitution and a specifically configured read loop.
  • For readability, avoid using i as a counter variable when there are semantically better options like file

src/woeusb Outdated
@@ -2004,6 +2009,19 @@ util_check_function_parameters_quantity(){
return 0
}; declare -fr util_check_function_parameters_quantity

##Check every file in source directory for files bigger than max fat32 file size (~4GB)
check_for_big_files(){
local source_fs_mountpoint="${1}"
Copy link
Collaborator

@brlin-tw brlin-tw May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/woeusb Outdated
@@ -2004,6 +2009,19 @@ util_check_function_parameters_quantity(){
return 0
}; declare -fr util_check_function_parameters_quantity

##Check every file in source directory for files bigger than max fat32 file size (~4GB)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pad a single space after the # symbol please

src/woeusb Outdated
for i in $(find ${source_fs_mountpoint}/ -type f); do
if (( $(du $i | cut -f -1) > 4194303 )); then
echo_with_color red "Error: File $i is larger than that supported by the fat32 filesystem. Use NTFS (--target-filesystem NTFS)."
exit 1
Copy link
Collaborator

@brlin-tw brlin-tw May 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return a non-zero value instead of directly bailout from a function using exit so that we can do more things in the caller function when we needed, for now, use the following snippet in the init function:

if ! check_for_big_files\
	"${source_fs_mountpoint}"; then
	exit 1
fi

WaxyMocha added 3 commits May 2, 2018 09:10
check_for_big_files returns status. Space after ##. file as counter variable. Comment explaining 4294967295. Read only variable source_fs_mountpoint.
src/woeusb Outdated
echo_with_color red "Error: File $i is larger than that supported by the fat32 filesystem. Use NTFS (--target-filesystem NTFS)."
exit 1
for file in $(find "${source_fs_mountpoint}" -type f); do
if (( $(stat -c "%s" $file) > 4294967295 )); then # Max fat32 file size is 2^32 - 1 bytes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process substitution will error when the path of file contains spaces, quote it with double-quotes.

@WaxyMocha
Copy link
Contributor Author

I checked it and it looks like it works

Error: File /home/waxy/win test/sources/ins ta ll.wim is larger than that supported by the fat32 filesystem. Use NTFS (--target-filesystem NTFS).

@brlin-tw
Copy link
Collaborator

brlin-tw commented May 2, 2018

I checked it and it looks like it works

Oh you've set the IFS, that's why the word separation isn't performed in the process substitution. You should backup and restore the original (default) IFS when tweaking it to avoid breaking future code that actually relies on it.

@WaxyMocha
Copy link
Contributor Author

Something like that?

local -r IFS_backup=$IFS
IFS=$'\n'
(..)
IFS=$IFS_backup

@brlin-tw
Copy link
Collaborator

brlin-tw commented May 2, 2018

@WaxyMocha
Exactly, but instead of setting the IFS_backup as read-only, unset it right after its purpose is done.

@brlin-tw brlin-tw merged commit 29d74ee into slacka:master May 2, 2018
@brlin-tw
Copy link
Collaborator

brlin-tw commented May 2, 2018

LGFM, thanks!

brlin-tw added a commit that referenced this pull request May 8, 2018
* Loop will break if filename contains newline(which is possible), use null-separated list instead.

Refer-to: For loops over find output are fragile. Use find -exec or a while read loop. - SC2044 · koalaman/shellcheck Wiki <https://github.com/koalaman/shellcheck/wiki/SC2044>
Signed-off-by: 林博仁(Buo-ren Lin) <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants