Conversation
corrodis
requested review from
Copilot,
eflumerf,
michaelmackenzie and
rrivera747
There was a problem hiding this comment.
Pull request overview
This PR refactors the get_artdaq_log_filenames method to improve command execution security and output parsing reliability. The changes modernize subprocess usage by replacing Popen with subprocess.run, eliminate shell injection vulnerabilities through proper command quoting, and introduce structured output parsing.
Changes:
- Modernized subprocess execution from
Popenwithshell=Truetosubprocess.runwith explicit argument lists - Improved SSH command security by using
shlex.quote()to prevent command injection - Simplified output parsing by replacing natural language format with structured markers (
__DAQLOG__N__)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "echo Logfile for process %s on %s is $filename_%s" | ||
| % (procinfo.label, procinfo.host, i_p) | ||
| ) | ||
| cmds.append("echo __DAQLOG__%s__ $filename_%s" % (i_p, i_p)) |
There was a problem hiding this comment.
This message appears to be much less informative...is there a reason for this?
There was a problem hiding this comment.
Actually, this is used for parsing to then match the correct logfile (that led to issues before). The key is to map the logfile to the i_p, this is what this is used for. The label and host are already known by artdaq-daqinterface. I added a log entry that combines all this (but not here).
Co-authored-by: eflumerf <61473357+eflumerf@users.noreply.github.com>
Use enumerate loop variable directly instead of re-indexing list
rrivera747
left a comment
rrivera747
left a comment
There was a problem hiding this comment.
@corrodis can you please resolve the copilot suggestions (it's fine to mark 'thumbs down' or 'ignore' and then click resolve). Thanks!
|
@rrivera747 all done and tested |
rrivera747
left a comment
rrivera747
left a comment
There was a problem hiding this comment.
Looks good to me - thanks!
No description provided.