judgedaemon: Replace system and other command executions.
#3156
Conversation
judge/judgedaemon.main.php
Outdated
| * | ||
| * @return bool Returns true on success (exit code 0), false otherwise. | ||
| */ | ||
| function run_command_safe(array $command_parts, ?int &$retval = null, $log_nonzero_exitcode = true): bool |
There was a problem hiding this comment.
When I look at most call sites, then $retval is not used afterwards, so it would be a bit simpler to be able to not pass it and then only within this function a local $retval variable is used.
There was a problem hiding this comment.
How do imagine that working with the code snippet where we use the $retval like this?
if (!isset($EXITCODES[$retval])) {
alert('error');
error("Unknown exitcode ($retval) from testcase_run.sh for s$judgeTask[submitid]");
}
$result = $EXITCODES[$retval];
There was a problem hiding this comment.
Inside run_command_safe I'd use a separate variable $retval_local and then assign that to $retval if it is passed, see below. Then you'd be able to call it as
run_command_safe(['echo', 'foobar']);when you don't care about the exit code, right?
There was a problem hiding this comment.
Got rid of $retval in the function call in more places, let me know if there is one more that I missed
judge/judgedaemon.main.php
Outdated
| $command = implode(' ', array_map('dj_escapeshellarg', $command_parts)); | ||
|
|
||
| logmsg(LOG_DEBUG, "Executing command: $command"); | ||
| system($command, $retval); |
There was a problem hiding this comment.
| system($command, $retval); | |
| system($command, $retval_local); | |
| if ( $retval!==null ) $retval = $retval_local; |
There was a problem hiding this comment.
This doesn't work directly because we cannot distinguish the different cases, replaced with a sentinel
judge/judgedaemon.main.php
Outdated
| * | ||
| * @return bool Returns true on success (exit code 0), false otherwise. | ||
| */ | ||
| function run_command_safe(array $command_parts, ?int &$retval = null, $log_nonzero_exitcode = true): bool |
There was a problem hiding this comment.
Inside run_command_safe I'd use a separate variable $retval_local and then assign that to $retval if it is passed, see below. Then you'd be able to call it as
run_command_safe(['echo', 'foobar']);when you don't care about the exit code, right?
meisterT
force-pushed
the
jh_saferun
branch
2 times, most recently
from
a5200b5 to
e246a89
Compare
| * | ||
| * @return bool Returns true on success (exit code 0), false otherwise. | ||
| */ | ||
| function run_command_safe(array $command_parts, &$retval = DONT_CARE, $log_nonzero_exitcode = true): bool |
There was a problem hiding this comment.
Why not
| function run_command_safe(array $command_parts, &$retval = DONT_CARE, $log_nonzero_exitcode = true): bool | |
| function run_command_safe(array $command_parts, ?int &$retval = null, $log_nonzero_exitcode = true): bool |
and just always assing to retval? What's the benefit of the don't care thingy?
There was a problem hiding this comment.
This is what I had before but then you cannot distinguish between not passing anything or passing a "freshly created" variable anymore. See also #3156 (comment)
There was a problem hiding this comment.
I don't think I understand: why do we want to distinguish? But if you thought about it, I'm fine with it of course.
There was a problem hiding this comment.
I thought it did break for compile errors, but when I just tried again to verify, it doesn't break. Probably I was holding it wrong before.
Introduce a safer and more readable variant that executes, logs and checks return values centrally.
3 participants
Introduce a safer and more readable variant that executes, logs and checks return values centrally.