Macintosh-HW/RASCSI
1
0
Fork 0
mirror of https://github.com/akuker/RASCSI.git synced 2025-08-05 01:27:46 +00:00
Code Issues Projects Releases Wiki Activity

More refactoring of Python code to address Sonar issues (#906)

Browse Source 
- Use Path objects for file operations
- Use urllib to sanitize URLs
- Some explicit type conversions
- Consistent regex syntax
- Add rudimentary logging when archive extraction caching fails
- Fixed two cases of the property file creation not being notified in the Flash message
- added doctype and html lang attribute to the web server 502 page
This commit is contained in:
Daniel Markstedt
2022-10-12 12:59:01 -07:00
committed by GitHub
parent dede2a6f35
commit 5a679509a1
5 changed files with 49 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
python/common/src/rascsi/file_cmds.py
View File

@@ -12,6 +12,7 @@ from time import time
from subprocess import run, CalledProcessError from subprocess import run, CalledProcessError
from json import dump, load from json import dump, load
from shutil import copyfile from shutil import copyfile
from urllib.parse import quote
import requests import requests
@@ -31,6 +32,7 @@ from util import unarchiver
FILE_READ_ERROR = "Unhandled exception when reading file: %s" FILE_READ_ERROR = "Unhandled exception when reading file: %s"
FILE_WRITE_ERROR = "Unhandled exception when writing to file: %s" FILE_WRITE_ERROR = "Unhandled exception when writing to file: %s"
URL_SAFE = "/:?&"
class FileCmds: class FileCmds:
""" """
@@ -103,7 +105,9 @@ class FileCmds:
for file in result.image_files_info.image_files: for file in result.image_files_info.image_files:
# Add properties meta data for the image, if applicable # Add properties meta data for the image, if applicable
if file.name in prop_files: if file.name in prop_files:
process = self.read_drive_properties(f"{CFG_DIR}/{file.name}.{PROPERTIES_SUFFIX}") process = self.read_drive_properties(
Path(CFG_DIR) / f"{file.name}.{PROPERTIES_SUFFIX}"
)
prop = process["conf"] prop = process["conf"]
else: else:
prop = False prop = False
@@ -377,7 +381,7 @@ class FileCmds:
tmp_full_path = tmp_dir + file_name tmp_full_path = tmp_dir + file_name
iso_filename = f"{server_info['image_dir']}/{file_name}.iso" iso_filename = f"{server_info['image_dir']}/{file_name}.iso"
req_proc = self.download_to_dir(url, tmp_dir, file_name) req_proc = self.download_to_dir(quote(url, safe=URL_SAFE), tmp_dir, file_name)
if not req_proc["status"]: if not req_proc["status"]:
return {"status": False, "msg": req_proc["msg"]} return {"status": False, "msg": req_proc["msg"]}
@@ -438,7 +442,11 @@ class FileCmds:
logging.info("Making a request to download %s", url) logging.info("Making a request to download %s", url)
try: try:
with requests.get(url, stream=True, headers={"User-Agent": "Mozilla/5.0"}) as req: with requests.get(
quote(url, safe=URL_SAFE),
stream=True,
headers={"User-Agent": "Mozilla/5.0"},
) as req:
req.raise_for_status() req.raise_for_status()
with open(f"{save_dir}/{file_name}", "wb") as download: with open(f"{save_dir}/{file_name}", "wb") as download:
for chunk in req.iter_content(chunk_size=8192): for chunk in req.iter_content(chunk_size=8192):
@@ -520,9 +528,9 @@ class FileCmds:
Takes (str) file_name Takes (str) file_name
Returns (dict) with (bool) status and (str) msg Returns (dict) with (bool) status and (str) msg
""" """
file_name = f"{CFG_DIR}/{file_name}" file_path = Path(CFG_DIR) / file_name
try: try:
with open(file_name, encoding="ISO-8859-1") as json_file: with open(file_path, encoding="ISO-8859-1") as json_file:
config = load(json_file) config = load(json_file)
# If the config file format changes again in the future, # If the config file format changes again in the future,
# introduce more sophisticated format detection logic here. # introduce more sophisticated format detection logic here.
@@ -584,7 +592,7 @@ class FileCmds:
logging.error(str(error)) logging.error(str(error))
return {"status": False, "msg": str(error)} return {"status": False, "msg": str(error)}
except: except:
logging.error(FILE_READ_ERROR, file_name) logging.error(FILE_READ_ERROR, str(file_path))
raise raise
def write_drive_properties(self, file_name, conf): def write_drive_properties(self, file_name, conf):
@@ -593,12 +601,12 @@ class FileCmds:
Takes file name base (str) and (list of dicts) conf as arguments Takes file name base (str) and (list of dicts) conf as arguments
Returns (dict) with (bool) status and (str) msg Returns (dict) with (bool) status and (str) msg
""" """
file_path = f"{CFG_DIR}/{file_name}" file_path = Path(CFG_DIR) / file_name
try: try:
with open(file_path, "w") as json_file: with open(file_path, "w") as json_file:
dump(conf, json_file, indent=4) dump(conf, json_file, indent=4)
parameters = { parameters = {
"target_path": file_path "target_path": str(file_path)
} }
return { return {
"status": True, "status": True,
@@ -607,25 +615,25 @@ class FileCmds:
} }
except (IOError, ValueError, EOFError, TypeError) as error: except (IOError, ValueError, EOFError, TypeError) as error:
logging.error(str(error)) logging.error(str(error))
self.delete_file(Path(file_path)) self.delete_file(file_path)
return {"status": False, "msg": str(error)} return {"status": False, "msg": str(error)}
except: except:
logging.error(FILE_WRITE_ERROR, file_path) logging.error(FILE_WRITE_ERROR, str(file_path))
self.delete_file(Path(file_path)) self.delete_file(file_path)
raise raise
# noinspection PyMethodMayBeStatic # noinspection PyMethodMayBeStatic
def read_drive_properties(self, file_path): def read_drive_properties(self, file_path):
""" """
Reads drive properties from json formatted file. Reads drive properties from json formatted file.
Takes (str) file_path as argument. Takes (Path) file_path as argument.
Returns (dict) with (bool) status, (str) msg, (dict) conf Returns (dict) with (bool) status, (str) msg, (dict) conf
""" """
try: try:
with open(file_path) as json_file: with open(file_path) as json_file:
conf = load(json_file) conf = load(json_file)
parameters = { parameters = {
"file_path": file_path "file_path": str(file_path)
} }
return { return {
"status": True, "status": True,
@@ -637,7 +645,7 @@ class FileCmds:
logging.error(str(error)) logging.error(str(error))
return {"status": False, "msg": str(error)} return {"status": False, "msg": str(error)}
except: except:
logging.error(FILE_READ_ERROR, file_path) logging.error(FILE_READ_ERROR, str(file_path))
raise raise
# noinspection PyMethodMayBeStatic # noinspection PyMethodMayBeStatic
@@ -673,5 +681,6 @@ class FileCmds:
""" """
try: try:
return unarchiver.inspect_archive(file_path) return unarchiver.inspect_archive(file_path)
except (unarchiver.LsarCommandError, unarchiver.LsarOutputError): except (unarchiver.LsarCommandError, unarchiver.LsarOutputError) as error:
logging.error(str(error))
raise raise

6
python/common/src/util/unarchiver.py
View File

@@ -69,7 +69,7 @@ def extract_archive(file_path, **kwargs):
unar_result_success = r'^Successfully extracted to "(?P<destination>.+)".$' unar_result_success = r'^Successfully extracted to "(?P<destination>.+)".$'
unar_result_no_files = "No files extracted." unar_result_no_files = "No files extracted."
unar_file_extracted = \ unar_file_extracted = \
r"^ (?P<path>.+). \(((?P<size>[0-9]+) B)?(?P<types>(dir)?(, )?(rsrc)?)\)\.\.\. (?P<status>[A-Z]+)\.$" r"^ {2}(?P<path>.+). \(((?P<size>\d+) B)?(?P<types>(dir)?(, )?(rsrc)?)\)\.\.\. (?P<status>[A-Z]+)\.$"
lines = process["stdout"].rstrip("\n").split("\n") lines = process["stdout"].rstrip("\n").split("\n")
@@ -141,7 +141,7 @@ def extract_archive(file_path, **kwargs):
# The parent dir may not be specified as a member, so ensure it exists # The parent dir may not be specified as a member, so ensure it exists
target_path.parent.mkdir(parents=True, exist_ok=True) target_path.parent.mkdir(parents=True, exist_ok=True)
logging.debug("Moving temp file: %s -> %s", source_path, target_path) logging.debug("Moving temp file: %s -> %s", source_path, target_path)
move(source_path, target_path) move(str(source_path), str(target_path))
moved.append(member) moved.append(member)
return { return {
@@ -152,7 +152,7 @@ def extract_archive(file_path, **kwargs):
raise UnarUnexpectedOutputError(lines[-1]) raise UnarUnexpectedOutputError(lines[-1])
def inspect_archive(file_path, **kwargs): def inspect_archive(file_path):
""" """
Calls `lsar` to inspect the contents of an archive Calls `lsar` to inspect the contents of an archive
Takes (str) file_path Takes (str) file_path

3
python/web/service-infra/502.html
View File

@@ -1,4 +1,5 @@
<html> <!doctype html>
<html lang="en">
<head> <head>
<title>RaSCSI-Web is Starting</title> <title>RaSCSI-Web is Starting</title>
<meta http-equiv="refresh" content="2"> <meta http-equiv="refresh" content="2">

21
python/web/src/web.py
View File

@@ -360,7 +360,11 @@ def drive_create():
if not process["status"]: if not process["status"]:
return response(error=True, message=process["msg"]) return response(error=True, message=process["msg"])
return response(message=_("Image file created: %(file_name)s", file_name=full_file_name)) # TODO: Refactor the return messages into one string
return response(message=[
(_("Image file created: %(file_name)s", file_name=full_file_name), "success"),
(process["msg"], "success"),
])
@APP.route("/drive/cdrom", methods=["POST"]) @APP.route("/drive/cdrom", methods=["POST"])
@@ -582,6 +586,7 @@ def attach_device():
if "interface" in params.keys(): if "interface" in params.keys():
bridge_status = is_bridge_configured(params["interface"]) bridge_status = is_bridge_configured(params["interface"])
if not bridge_status["status"]: if not bridge_status["status"]:
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(bridge_status["msg"], "error"), (bridge_status["msg"], "error"),
(error_msg, "error") (error_msg, "error")
@@ -673,6 +678,7 @@ def attach_image():
return response(message=response_messages) return response(message=response_messages)
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to attach %(file_name)s to SCSI ID %(id_number)s LUN %(unit_number)s", (_("Failed to attach %(file_name)s to SCSI ID %(id_number)s LUN %(unit_number)s",
file_name=file_name, id_number=scsi_id, unit_number=unit), "error"), file_name=file_name, id_number=scsi_id, unit_number=unit), "error"),
@@ -706,6 +712,7 @@ def detach():
return response(message=_("Detached SCSI ID %(id_number)s LUN %(unit_number)s", return response(message=_("Detached SCSI ID %(id_number)s LUN %(unit_number)s",
id_number=scsi_id, unit_number=unit)) id_number=scsi_id, unit_number=unit))
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to detach SCSI ID %(id_number)s LUN %(unit_number)s", (_("Failed to detach SCSI ID %(id_number)s LUN %(unit_number)s",
id_number=scsi_id, unit_number=unit), "error"), id_number=scsi_id, unit_number=unit), "error"),
@@ -727,6 +734,7 @@ def eject():
return response(message=_("Ejected SCSI ID %(id_number)s LUN %(unit_number)s", return response(message=_("Ejected SCSI ID %(id_number)s LUN %(unit_number)s",
id_number=scsi_id, unit_number=unit)) id_number=scsi_id, unit_number=unit))
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to eject SCSI ID %(id_number)s LUN %(unit_number)s", (_("Failed to eject SCSI ID %(id_number)s LUN %(unit_number)s",
id_number=scsi_id, unit_number=unit), "error"), id_number=scsi_id, unit_number=unit), "error"),
@@ -764,6 +772,7 @@ def reserve_id():
RESERVATIONS[int(scsi_id)] = memo RESERVATIONS[int(scsi_id)] = memo
return response(message=_("Reserved SCSI ID %(id_number)s", id_number=scsi_id)) return response(message=_("Reserved SCSI ID %(id_number)s", id_number=scsi_id))
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to reserve SCSI ID %(id_number)s", id_number=scsi_id), "error"), (_("Failed to reserve SCSI ID %(id_number)s", id_number=scsi_id), "error"),
(process["msg"], "error"), (process["msg"], "error"),
@@ -784,6 +793,7 @@ def release_id():
RESERVATIONS[int(scsi_id)] = "" RESERVATIONS[int(scsi_id)] = ""
return response(message=_("Released the reservation for SCSI ID %(id_number)s", id_number=scsi_id)) return response(message=_("Released the reservation for SCSI ID %(id_number)s", id_number=scsi_id))
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to release the reservation for SCSI ID %(id_number)s", id_number=scsi_id), "error"), (_("Failed to release the reservation for SCSI ID %(id_number)s", id_number=scsi_id), "error"),
(process["msg"], "error"), (process["msg"], "error"),
@@ -824,6 +834,7 @@ def download_to_iso():
process = file_cmd.download_file_to_iso(url, *iso_args) process = file_cmd.download_file_to_iso(url, *iso_args)
process = ReturnCodeMapper.add_msg(process) process = ReturnCodeMapper.add_msg(process)
if not process["status"]: if not process["status"]:
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to create CD-ROM image from %(url)s", url=url), "error"), (_("Failed to create CD-ROM image from %(url)s", url=url), "error"),
(process["msg"], "error"), (process["msg"], "error"),
@@ -842,6 +853,7 @@ def download_to_iso():
response_messages.append((_("Attached to SCSI ID %(id_number)s", id_number=scsi_id), "success")) response_messages.append((_("Attached to SCSI ID %(id_number)s", id_number=scsi_id), "success"))
return response(message=response_messages) return response(message=response_messages)
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to attach image to SCSI ID %(id_number)s. Try attaching it manually.", id_number=scsi_id), "error"), (_("Failed to attach image to SCSI ID %(id_number)s. Try attaching it manually.", id_number=scsi_id), "error"),
(process_attach["msg"], "error"), (process_attach["msg"], "error"),
@@ -866,6 +878,7 @@ def download_file():
if process["status"]: if process["status"]:
return response(message=process["msg"]) return response(message=process["msg"])
# TODO: Refactor the return messages into one string
return response(error=True, message=[ return response(error=True, message=[
(_("Failed to download file from %(url)s", url=url), "error"), (_("Failed to download file from %(url)s", url=url), "error"),
(process["msg"], "error"), (process["msg"], "error"),
@@ -922,7 +935,11 @@ def create_file():
return response( return response(
status_code=201, status_code=201,
message=_("Image file created: %(file_name)s", file_name=full_file_name), # TODO: Refactor the return messages into one string
message=[
(_("Image file created: %(file_name)s", file_name=full_file_name), "success"),
(process["msg"], "success"),
],
image=full_file_name, image=full_file_name,
) )

1
python/web/src/web_utils.py
View File

@@ -240,6 +240,7 @@ def is_bridge_configured(interface):
Takes (str) interface of a network device being attached. Takes (str) interface of a network device being attached.
Returns a (dict) with (bool) status and (str) msg Returns a (dict) with (bool) status and (str) msg
""" """
# TODO: Reduce the nesting of these checks, and streamline how the results are notified
status = True status = True
return_msg = "" return_msg = ""
sys_cmd = SysCmds() sys_cmd = SysCmds()