Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OIL: introduce internal_error(fmt) for error reporting #5334

Merged
merged 1 commit into from
Jan 5, 2024

Conversation

lindig
Copy link
Contributor

@lindig lindig commented Jan 5, 2024

Simplify the logging and reporting of internal errors in xenopsd. Rather than constructing a complex exception use a function that accepts a printf-style string for convenience. It logs the error and raises the exception.

@@ -2731,10 +2727,9 @@ and perform_exn ?subtask ?result (op : operation) (t : Xenops_task.task_handle)
the destination host. even though the destination host
failed to respond successfully to our handshake, the VM
should still be running correctly *)
error
internal_error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the string of the exception compared to the old code. But it just adds more info to the message, which should be fine.

in
error "%s: %s" __FUNCTION__ err_msg ;
raise (Xenopsd_error (Internal_error err_msg))
internal_error "More than one domain with uuid %s: (%s)" uuid' domid_list
| exception Failure r ->
raise (Xenopsd_error (Internal_error r))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do this one as well?

@@ -30,6 +30,14 @@ let finally = Xapi_stdext_pervasives.Pervasiveext.finally

let with_lock = Xapi_stdext_threads.Threadext.Mutex.execute

let internal_error fmt =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have 2 copies of this, I assume because the 'error' function is different (it includes the module name).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed; otherwise you need to make sure it's logged under the correct name.

Copy link
Contributor

@Vincent-lau Vincent-lau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original code has lots of duplications and nestings 🙅

One question: if we raise, then that gets logged anyway, do we need to err to log and then raise as well?

@lindig
Copy link
Contributor Author

lindig commented Jan 5, 2024

In my new code I log the error just to be very close to the origin of that error and because I don't want to rely on top-level code that might or might not log the error. But you are right that we could decide not to log at this point.

Simplify the logging and reporting of internal errors in xenopsd. Rather
than constructing a complex exception use a function that accepts a
printf-style string for convenience. It logs the error and raises the
exception.

Signed-off-by: Christian Lindig <christian.lindig@cloud.com>
@lindig
Copy link
Contributor Author

lindig commented Jan 5, 2024

I was too eager in one case and changed the exception - fixed that back to the original code:

-            let m =
-              Printf.sprintf
-                "VM = %s; domid = %d; Bootloader.Error_from_bootloader %s"
-                vm.Vm.id domid x
-            in
-            debug "%s" m ;
-            raise (Xenopsd_error (Bootloader_error (vm.Vm.id, x)))
+            internal_error
+              "VM = %s; domid = %d; Bootloader.Error_from_bootloader %s"
+              vm.Vm.id domid x

@lindig lindig merged commit 730dc28 into xapi-project:master Jan 5, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants