From 0141a6e22952c82ea138943ac1ac7dcf4a6b367c Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Wed, 4 Oct 2023 12:34:42 +0200 Subject: [PATCH 1/4] getDiskDeviceSize: don't silently return None (part of #66) Signed-off-by: Yann Dirson --- diskutil.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/diskutil.py b/diskutil.py index 588f4379..101ccb02 100644 --- a/diskutil.py +++ b/diskutil.py @@ -280,6 +280,9 @@ def getDiskDeviceSize(dev): return int(__readOneLineFile__("/sys/block/%s/device/block/size" % dev)) elif os.path.exists("/sys/block/%s/size" % dev): return int(__readOneLineFile__("/sys/block/%s/size" % dev)) + else: + raise Exception("%s not found as %s or %s" % (dev, "/sys/block/%s/device/block/size", + "/sys/block/%s/size")) def getDiskSerialNumber(dev): # For Multipath nodes return info about 1st slave From 1781d8817f7fef0cf3d23f70501f3e10662005df Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Wed, 4 Oct 2023 15:41:56 +0200 Subject: [PATCH 2/4] findXenSourceBackups: log any exception caught - it is bad practice to "catch any" - not logging anything just hides information, which can be especially important here as the reason for a try/catch is not obvious (exceptions thrown by XenServerBackup.__init__?) Signed-off-by: Yann Dirson --- product.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/product.py b/product.py index 5f42f38c..61eb0c99 100644 --- a/product.py +++ b/product.py @@ -540,7 +540,8 @@ def findXenSourceBackups(): if backup.version >= XENSERVER_MIN_VERSION and \ backup.version <= THIS_PLATFORM_VERSION: backups.append(backup) - except: + except Exception as ex: + logger.log("findXenSourceBackups caught exception for partition %s: %s" % (p, ex)) pass if b: b.unmount() From c3f126eaa6515be3b5c4c5cba619c1b354e48113 Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Wed, 4 Oct 2023 14:48:31 +0200 Subject: [PATCH 3/4] findXenSourceBackups: refactor and log reason for ignoring Previous code structure was that of 2 nested if's, with nominal code path being in the positive branch of each if. Adding another condition will bring a need for logging the reason to ignore a backup, but doing that by converting inner `if` to an `elif` chain (arguably the simplest modification) would bring the nominal code path to switch to the negative/last branch of the inner `elif` chain. At the same time, the outer `if` would not seem special enough to deserve it special place (and the cyclomatic complexity). This commit leverages the existing `try:except:` block to switch to an "error out the loop" pattern, where the nominal code path is just linear. Using `StopIteration` may feel like an abuse, but: - it is the only standard `Exception` that is not a `StandardError` or a `Warning`, and defining a new one could seem overkill - the closest alternative would be a `while True` loop and breaking out in both exceptional code paths and at the end of nominal path, where the `break` statements would "stop the iteration" as well Signed-off-by: Yann Dirson --- product.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/product.py b/product.py index 61eb0c99..0a919365 100644 --- a/product.py +++ b/product.py @@ -534,12 +534,25 @@ def findXenSourceBackups(): b = None try: b = util.TempMount(p, 'backup-', ['ro'], 'ext3') - if os.path.exists(os.path.join(b.mount_point, '.xen-backup-partition')): - backup = XenServerBackup(p, b.mount_point) - logger.log("Found a backup: %s" % (repr(backup),)) - if backup.version >= XENSERVER_MIN_VERSION and \ - backup.version <= THIS_PLATFORM_VERSION: - backups.append(backup) + if not os.path.exists(os.path.join(b.mount_point, '.xen-backup-partition')): + raise StopIteration() + + backup = XenServerBackup(p, b.mount_point) + logger.log("Found a backup: %s" % (repr(backup),)) + + if backup.version < XENSERVER_MIN_VERSION: + logger.log("findXenSourceBackups: ignoring, platform too old: %s < %s" % + (backup.version, XENSERVER_MIN_VERSION)) + raise StopIteration() + if backup.version > THIS_PLATFORM_VERSION: + logger.log("findXenSourceBackups: ignoring later platform: %s > %s" % + (backup.version, THIS_PLATFORM_VERSION)) + raise StopIteration() + + backups.append(backup) + + except StopIteration: + pass except Exception as ex: logger.log("findXenSourceBackups caught exception for partition %s: %s" % (p, ex)) pass From 7a993355d6d9f043eba4dc1c6454c48d32027495 Mon Sep 17 00:00:00 2001 From: Yann Dirson Date: Wed, 4 Oct 2023 14:25:35 +0200 Subject: [PATCH 4/4] XenServerBackup: ignore backups with inaccessible primary disk (#66) Signed-off-by: Yann Dirson --- product.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/product.py b/product.py index 0a919365..27a7eb1a 100644 --- a/product.py +++ b/product.py @@ -548,6 +548,10 @@ def findXenSourceBackups(): logger.log("findXenSourceBackups: ignoring later platform: %s > %s" % (backup.version, THIS_PLATFORM_VERSION)) raise StopIteration() + if not os.path.exists(backup.root_disk): + logger.error("findXenSourceBackups: PRIMARY_DISK=%r does not exist" % + (backup.root_disk,)) + raise StopIteration() backups.append(backup)