fix(FileSR): apply review suggestions for vdi_revert

Signed-off-by: Antoine Bartuccio <antoine.bartuccio@vates.tech>

Author: klmp200

drivers/FileSR.py

@@ -260,8 +260,8 @@ def _undo_all_journals(self):
         if 'SRmaster' in self.dconf and self.dconf['SRmaster'] == 'true':
             is_master = True
 
-        if self.cmd in ["vdi_detach", "vdi_activate", "vdi_deactivate", "nop"]:
-            return
+        # if self.cmd in ["vdi_detach", "vdi_activate", "vdi_deactivate", "nop"]:
+        #     return
 
         if not is_master:
             raise xs_errors.XenError(
@@ -530,13 +530,13 @@ def _rollback_revert_vdi(self, entry: RevertLogEntry):
         if not util.ioretry(lambda: util.pathexists(entry.dest.backup_path)):
             util.SMlog(f"Tried reverting {entry.dest.uuid} but backup {entry.dest.backup_path} not found, skpping")
             return
+
         src_cowutil = getCowUtil(entry.src.vdi_type)
         util.SMlog(f"Restoring src vdi {entry.src.uuid}")
         if not util.ioretry(lambda: util.pathexists(entry.src.path)):
             util.SMlog(f"Restoring src vdi {entry.src.uuid} from {entry.inserted.uuid}")
             util.ioretry(lambda: os.rename(entry.inserted.path, entry.src.path))
 
-        util.SMlog(f"Restoring src vdi {entry.src.uuid}")
         src_cowutil.setHidden(entry.src.path, False)
 
         src: "FileVDI" = self.vdi(entry.src.uuid) # type: ignore[assignment]
@@ -596,7 +596,7 @@ def _unlink_paths(self, *files: Union[Path, str]):
     def _handle_revert_journals(self):
         for uuid, value in self.journaler.getAll(RevertLogEntry.JRN_KEY).items():
             entry = RevertLogEntry.from_journal(uuid, value)
-            util.SMlog(f"Reverting {entry.dest.uuid}")
+            util.SMlog(f"Reverting journal {entry}")
 
             self._rollback_revert_vdi(entry)
 
@@ -1062,19 +1062,8 @@ def _revert(
 
         util.fistpoint.activate("FileSR_revert_create_insert", self.sr.uuid)
 
-        # Create inserted base copy
-        util.ioretry(
-            lambda: self._snap(
-                self.path,
-                inserted_path,
-                False,
-            )
-        )
-
         inserted = FileVDI(self.sr, inserted_uuid)
 
-        util.fistpoint.activate("FileSR_revert_create_src", self.sr.uuid)
-
         inserted.label = "base copy"
         inserted.read_only = True
         inserted.location = inserted_uuid
@@ -1086,6 +1075,17 @@ def _revert(
 
         inserted._db_introduce()
 
+        util.fistpoint.activate("FileSR_revert_create_src", self.sr.uuid)
+
+        # Create src again
+        util.ioretry(
+            lambda: self._snap(
+                self.path,
+                inserted_path,
+                False,
+            )
+        )
+
         # Update src
         self.load_from_file(self.path)
         self._db_update()
@@ -1099,16 +1099,17 @@ def _revert(
             )
         )
         self.cowutil.setHidden(dest.path, False)
-        ## Force parent (it can be simplified by vhd tools)
-        self.cowutil.setParent(dest.path, inserted.path, False)
 
         util.fistpoint.activate("FileSR_revert_create_dest", self.sr.uuid)
 
+        dest.load_from_file(dest.path)
+        dest._db_update()
+
         ## The new parent can now be set hidden and don't need to be protected by the GC
         inserted.cowutil.setHidden(inserted.path, True)
+        inserted.load_from_file(inserted.path)
 
-        dest.load_from_file(dest.path)
-        dest._db_update()
+        inserted._db_update()
 
         if src_cbtlog:
             self._revert_cbt(dest)

drivers/SRCommand.py

@@ -273,7 +273,7 @@ def _run(self, sr, target):
         elif self.cmd == 'vdi_revert':
             dest_uuid = self.params['args'][0]
             if dest_uuid.startswith("OpaqueRef:"):
-                # TODO: remove once xapi sends an uuid instead of an OpaqueRef
+                # TODO(XCPNG-3486): remove once xapi sends an uuid instead of an OpaqueRef
                 dest_uuid = sr.session.xenapi.VDI.get_record(dest_uuid)["uuid"]
             return target.revert(
                 self.params['sr_uuid'],

drivers/VDI.py

@@ -256,7 +256,7 @@ def resize(self, sr_uuid, vdi_uuid, size) -> str:
         """
         raise xs_errors.XenError('Unimplemented')
 
-    def revert(self, sr_uuid: str, vdi_uuid: str, target_uuid: str) -> None:
+    def revert(self, _sr_uuid: str, _vdi_uuid: str, target_uuid: str) -> None:
         """Replaces the contents of the target_uuid VDI with the contents of the vdi_uuid
         without changing the identitity of the target (i.e. name-label, uuid and location
         are guaranteed to remain the same)..
@@ -964,11 +964,9 @@ def _disable_cbt_on_vdi(self, vdi: "VDI", alert_name: str, alert_str: str) -> No
         vdi._delete_cbt_log()
         vdi_ref = vdi.session.xenapi.VDI.get_by_uuid(vdi.uuid)
         vdi.session.xenapi.VDI.set_cbt_enabled(vdi_ref, False)
-        alert_prio_warning = "3"
-        alert_obj = "VDI"
         alert_uuid = str(vdi.uuid)
         vdi.session.xenapi.message.create(
-            alert_name, alert_prio_warning, alert_obj, alert_uuid, alert_str
+            alert_name, "3", "VDI", alert_uuid, alert_str
         )
 
     def _create_cbt_log_with_size(self, size: int) -> str:

drivers/jutils.py

@@ -1,6 +1,6 @@
 #!/usr/bin/python3
 #
-# Copyright (C) 2020  Vates SAS - antoine.bartuccio@vates.fr
+# Copyright (C) 2026  Vates SAS - antoine.bartuccio@vates.fr
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -53,6 +53,8 @@ def _get_version_from_journal_id(journal_id: str) -> str:
 
     @classmethod
     def from_journal(cls: Type[LogEntry], journal_id: str, value: str) -> LogEntry:
+        """Convert a compatilbe journal to a log entry
+        Ensure that the type and version of the journal are compatible"""
         version = cls._get_version_from_journal_id(journal_id)
         if version != cls.CURRENT_VERSION:
             raise xs_errors.SRException(