Add ronin compress command
#184
Conversation
|
@postmodern 0.1 version of this PR. |
| # | ||
| # [FILE ...] Optional file(s) to compress | ||
| # | ||
| class Compress < StringMethodsCommand |
There was a problem hiding this comment.
If we only want to support processing files, then probably want to inherit from FileProcessorCommand, and define a process_input(io) method to compress the input File or from stdin.
If we do want to support accepting arbitrary Strings (ex: ronin compress --string "hello world"), then we should inherit from StringProcessorCommand.
StringMethodsCommand is more meant to be used by commands which simply map options to certain monkey-patch methods that are added to String (ex: ronin decode --base64 -> String#base64_decode).
lib/ronin/cli/commands/compress.rb
Outdated
| option :gzip, short: '-g', | ||
| desc: 'gzip compresses the data' do | ||
| @format = :gzip | ||
| end |
There was a problem hiding this comment.
If we're going to support --gzip, we should also support a --zlib option. ronin-support also happens to have a Ronin::Support::Compression::Zlib module we can use.
lib/ronin/cli/commands/compress.rb
Outdated
| # | ||
| # The compression format. | ||
| # | ||
| # @return Symbol |
There was a problem hiding this comment.
I tend to explicitly list the possible values @return [:gzip, :tar, :zip].
lib/ronin/cli/commands/compress.rb
Outdated
| end | ||
|
|
||
| def extension | ||
| return "gz" if @format == :gzip |
There was a problem hiding this comment.
We could probably set an @ext instance variable in the option blocks, along with @format. This way we follow the "Tell, don't ask" principle.
lib/ronin/cli/commands/compress.rb
Outdated
| end | ||
|
|
||
| def process_file(file) | ||
| compression_class.send(@format, "#{file}.#{extension}") do |c| |
There was a problem hiding this comment.
Maybe rename @format to @method or @compression_method if we're going to send() it.
lib/ronin/cli/commands/compress.rb
Outdated
|
|
||
| def process_file(file) | ||
| compression_class.send(@format, "#{file}.#{extension}") do |c| | ||
| c.write File.read(file) |
There was a problem hiding this comment.
The files being compressed could be very very large. Probably more efficient to open the input file and read chunks of data from it using input.readpartial(4096) and write those to the output stream.
lib/ronin/cli/commands/compress.rb
Outdated
| end | ||
|
|
||
| def process_file(file) | ||
| compression_class.send(@format, "#{file}.#{extension}") do |c| |
There was a problem hiding this comment.
So here's a question, if you gave this command the --zip option and multiple files, would the user expect each file to be compressed into it's own .zip file, or all files to be compressed into one zip file?
|
@AI-Mozi what do you think about adding separate |
|
It's definitely a good idea! |
lib/ronin/cli/commands/compress.rb
Outdated
| option :name, short: '-n', | ||
| value: { | ||
| type: String | ||
| }, | ||
| desc: 'compressed file name' do |name| | ||
| @compressed_file_name = name | ||
| end |
There was a problem hiding this comment.
I think we should add an option for specifying compressed file name.
lib/ronin/cli/commands/compress.rb
Outdated
| # The path to the file. | ||
| # | ||
| def process_file(file) | ||
| raise "Files can be compressed using gzip only" if @compression_method != :gzip |
There was a problem hiding this comment.
Should we raise an error, or just print some message and return?
There was a problem hiding this comment.
Call print_error and exit(1), or call print_error and return if you think the error can be printed but the command should continue processing arguments.
lib/ronin/cli/commands/compress.rb
Outdated
| rescue EOFError | ||
| rescue IOError => e |
There was a problem hiding this comment.
Rubocop does not like empty rescue :/
There was a problem hiding this comment.
Ideally, there should be no IO errors when reading the file. I suspect file.eof? should make the rescues unnecessary.
lib/ronin/cli/commands/compress.rb
Outdated
|
|
||
| Ronin::Support::Compression.gzip(compressed_file_name) do |gz| | ||
| files.each do |file| | ||
| File.open(file, 'rb') do |f| |
There was a problem hiding this comment.
Avoid single letter block arguments. Rename file to like path or file_path, and f to file or io.
lib/ronin/cli/commands/compress.rb
Outdated
| rescue EOFError | ||
| rescue IOError => e |
There was a problem hiding this comment.
Ideally, there should be no IO errors when reading the file. I suspect file.eof? should make the rescues unnecessary.
lib/ronin/cli/commands/compress.rb
Outdated
| if files.empty? | ||
| super(*files) | ||
| else | ||
| raise "Files can be compressed using gzip only" if @compression_method != :gzip |
There was a problem hiding this comment.
Do a print_error and call exit(1) if invalid combinations of options or arguments are given.
lib/ronin/cli/commands/compress.rb
Outdated
| # The compressed string. | ||
| # | ||
| def process_string(string) | ||
| string.send(@compression_method) |
There was a problem hiding this comment.
I try to avoid calling the monkey-patched methods when possible. Might be better to do Support::Compression.send(@compression_method,string).
lib/ronin/cli/commands/compress.rb
Outdated
| # @return String | ||
| # | ||
| def compressed_file_name | ||
| @compressed_file_name || "ronin_compressed_#{Time.now.to_i}.gz" |
There was a problem hiding this comment.
I'd get rid of the timestamp, or maybe add an option for it. I think a ronin compress command should behave exactly like gzip and just append the .gz extension on the given file name.
lib/ronin/cli/commands/compress.rb
Outdated
| # The path to the file. | ||
| # | ||
| def process_file(file) | ||
| raise "Files can be compressed using gzip only" if @compression_method != :gzip |
There was a problem hiding this comment.
Call print_error and exit(1), or call print_error and return if you think the error can be printed but the command should continue processing arguments.
lib/ronin/cli/commands/compress.rb
Outdated
| if files.empty? | ||
| super(*files) | ||
| else | ||
| raise "Files can be compressed using gzip only" if @compression_method != :gzip |
There was a problem hiding this comment.
I think we could support compressing files using SupportCompression.zlib_deflate. Just read the entire file using File.read, compress the String, and write it back out using File.binwrite.
lib/ronin/cli/commands/compress.rb
Outdated
| else | ||
| raise "Files can be compressed using gzip only" if @compression_method != :gzip | ||
|
|
||
| Ronin::Support::Compression.gzip(compressed_file_name) do |gz| |
There was a problem hiding this comment.
Also why are there two Ronin::Support::Compression.gzip(compressed_file_name) blocks? One in run and another in process_file?
lib/ronin/cli/commands/compress.rb
Outdated
| # File arguments. | ||
| # | ||
| def run(*files) | ||
| if files.empty? |
There was a problem hiding this comment.
Shouldn't this be unless files.empty? or if !files.empty??
There was a problem hiding this comment.
I thought about it like if there is file given as an argument, compress it, otherwise go to StringProcessorCommand#run else statement and process @input_values.
There was a problem hiding this comment.
Ohh.. now that I read it, it works different than i thought 😅
There was a problem hiding this comment.
Hmm maybe it might be easier to just write all files and strings into a common Support::Compression::GZip output stream? Although, we would have to have different logic for the Compression::Zlib code.
Another idea would be to define separate process_file and process_string methods. The process_file method would read the File and write the gziped/zlib'ed output to a .gz or .zlib file. The process_string method would instead gzip/zlib compress a String and write it to stdout; maybe also support a common --output option?
|
@postmodern It's been a while. I did some changes. Let me know what do you thing and we can discuss that later :D |
|
@AI-Mozi I'm not sure about how the I do however think there should be a single |
#145