From a6a3a8f9dc516c226fba54bc7dd1730573dfcbd2 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..186c56cc 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("{0} not found as /sys/block/{0}/device/block/size or /sys/block/{0}/size" + .format(dev)) def getDiskSerialNumber(dev): # For Multipath nodes return info about 1st slave From 6614456111584c113fa7ec78ac31edf2d4950ecf 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/product.py b/product.py index 5f42f38c..d4123a5f 100644 --- a/product.py +++ b/product.py @@ -540,7 +540,9 @@ 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" % (p,)) + logger.logException(ex) pass if b: b.unmount() From b27040837bdb941dfc383ed06d53da7d939277e9 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 d4123a5f..1fb1e0a5 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" % (p,)) logger.logException(ex) From 44624db89b574c9e9e7203ec52f95b6778a3ad93 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 1fb1e0a5..3c6a3682 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)