-
Notifications
You must be signed in to change notification settings - Fork 15
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
Graceful, Automatic Expired Key Handling #121
Conversation
… (I know, I know...)
Added log for successful Blu-ray test |
Any ETA on a merge or response? Multiple tests against Blu-ray and DVD disks have been successful |
This means that Could you bump the VERSION number up ? Thanks, and great work! |
This reverts commit 2e73b5d.
Code Climate has analyzed commit bcd3630 and detected 0 issues on this pull request. View more on Code Climate. |
Kudos, SonarCloud Quality Gate passed! |
|
Lemme know when v2.6.0 has been tagged and released Edit: merge #123 before tagging and releasing v2.6.0 |
Overview
Manually updating the key yet again this morning really bothered me, so I figured I'd take a look at how the key is stored by MakeMKV, and look around on the forums to see what others came up with. Turns out it's stored plaintext in
$HOME/.MakeMKV/settings.conf
and looks like this:So scraping and storing the key is now handled by
scripts/update_key.sh
.Actually leveraging the new script was another matter entirely, and required unsnarling the monolithic mess that has been in there since the beginning of the upstream project.
The logic to run the shell commands and handle exceptions that arose existed in 5 different places (three of those instances were exact duplicates of ~18 lines). Those 3 duplicates have now been moved into the function
run_makemkv(cmd)
, which takes a shell command. No more chunks of duplicated code makingmakemkv()
hard to read.I fixed the error handilng.
subprocess.run()
doesn't actually throw thesubprocess.CalledProcessError
that the code tries to catch unless it looks likesubprocess.run(..., check=True)
. So I standardized that as well, and now the exceptions properly catch all non-zero error codes (more on this later).I have completely discarded handling the exception thrown when MakeMKV errors out on an expired key once arm enters the command crafting logic. Instead, the first thing
makemkv()
does is call the newprep_mkv(job)
function. It takes the current job and runsmakemkvcon info job.devpath
. Normally this doesn't do anything, and provides no use.But, after the expiration of the key (regardless of what time of day MakeMKV's servers stop recognizing the previous month's key), it errors out with error code
253
. The neat part is that doing this here allows us to handle key expiration in one place, well before entry into sensitive logic. So now we've gone from 5 locations needing handling for key expiration to 1.Once error code
253
is caught it calls the newupdate_key()
function which runsscripts/update_key.sh
and throws a RuntimeError that kills the job if it encounters an error during execution. Onceupdate_key()
has successfully returned, then the exception handling tries runningmakemkvcon info job.devpath
again. For some weird reason, a successful run of that command has a return code of 10, so the handling throws an exception and kills the job on any non-zero return code that is not10
.Once the second run of
makemkvcon info job.devpath
is completed successfully, the exception handling returns without propagating the exception. Logging documents several steps of this process, and the rip continues as if nothing happened.Exceptions. By this point I had half a dozen instances or the same three lines of code that made up the entirety of the except body of the except block. So I moved that logic into a custom exception:
MakeMkvRuntimeException
. It takes thesubprocess.CalledProcessError
variable and uses that to craft a useful message. So everything looking like this:Becomes this:
Overall, this makes the code cleaner, more concise, and significantly easier to read. In particular,
makemkv()
has gone from an enormous 161 SLoC to 101 SLoC. Additionally, all of the new functions, exception class, and docstrings for them all only increases the total size ofripper/makemkv.py
by 2 SLoC!Changelog
ripper/makekmv.py
:MakeMkvRuntimeError
ripper/makekmv.py
:prep_makemkv()
,update_key()
,run_make()
makemkv()
scripts/update_key.sh
to scrape and store the latest beta keyNotes
$HOME/.MakeMKV/settings.conf
(for me it was/home/arm/.MakeMKV/settings.conf
). All of the logging for this functionality is INFO levelNext Steps
v2.6.0
. I am planning to do so in my own fork once this is mergedsudo chmod +x /opt/arm/scripts/update_key.sh
following thesudo chmod +x /opt/arm/scripts/arm_wrapper.sh
Logs
Attached is the log from my dvd test run:
POINT_BREAK_164679631972.log
KIKIS_DELIVERY_SERVICE_BD_164694558691.log