Hey,
This one seems much simpler to review once split into something like
(split was very rough and most likely only partial) the
3 attached patches.
Christophe
On Thu, Nov 13, 2014 at 06:20:42PM +0100, Marc-André Lureau wrote:
> Some refactoring to make the code easier to read.
> - do not overwrite err if ->initial_connect() sets it
> - remove need for waitvm if the display server isn't yet started (note:
> this function might be untested, I am not sure relying on libvirt events
> is enough)
> ---
> src/virt-viewer.c | 37 +++++++++++++++----------------------
> 1 file changed, 15 insertions(+), 22 deletions(-)
>
> diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> index 6810908..c1d2765 100644
> --- a/src/virt-viewer.c
> +++ b/src/virt-viewer.c
> @@ -641,7 +641,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
> if (!priv->conn &&
> virt_viewer_connect(app) < 0) {
> virt_viewer_app_show_status(app, _("Waiting for libvirt to start"));
> - goto done;
> + goto wait;
> }
>
> virt_viewer_app_show_status(app, _("Finding guest domain"));
> @@ -649,9 +649,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
> if (!dom) {
> if (priv->waitvm) {
> virt_viewer_app_show_status(app, _("Waiting for guest domain to be created"));
> - virt_viewer_app_trace(app, "Guest %s does not yet exist, waiting for it to be created",
> - priv->domkey);
> - goto done;
> + goto wait;
> } else {
> dom = choose_vm(&priv->domkey, priv->conn, &err);
> if (dom == NULL && err != NULL) {
> @@ -675,27 +673,22 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
>
> if (info.state == VIR_DOMAIN_SHUTOFF) {
> virt_viewer_app_show_status(app, _("Waiting for guest domain to start"));
> - } else {
> - ret = virt_viewer_update_display(self, dom);
> - if (ret)
> - ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
> - if (!ret) {
> - if (priv->waitvm) {
> - virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
> - virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting for it to start",
> - priv->domkey);
> - } else {
> - g_set_error_literal(&err, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> - _("Failed to activate viewer"));
> - g_debug("%s", err->message);
> - goto cleanup;
> - }
> - }
> + goto wait;
> }
>
> - done:
> + if (!virt_viewer_update_display(self, dom))
> + goto wait;
> +
> + ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
> + if (ret || err)
> + goto cleanup;
> +
> +wait:
> + virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting "
> + "for it to start", priv->domkey);
> ret = TRUE;
> - cleanup:
> +
> +cleanup:
> if (err != NULL)
> g_propagate_error(error, err);
> if (dom)
> --
> 1.9.3
>
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
From 587da1164e4c93fc51ff00dcae71e61c02e1d374 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@xxxxxxxxx>
Date: Thu, 13 Nov 2014 18:20:42 +0100
Subject: [virt-viewer 1/3] Simplify virt_viewer_initial_connect()
Some refactoring to make the code easier to read, mostly code
movement/reindenting and introduction of a "wait" label which has the
same purpose as "done".
This also adds a "goto wait" within an if block, but this does not
change the initial code flow, just makes it more explicit.
---
src/virt-viewer.c | 42 +++++++++++++++++++++---------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/src/virt-viewer.c b/src/virt-viewer.c
index 6810908..fe77d34 100644
--- a/src/virt-viewer.c
+++ b/src/virt-viewer.c
@@ -641,7 +641,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
if (!priv->conn &&
virt_viewer_connect(app) < 0) {
virt_viewer_app_show_status(app, _("Waiting for libvirt to start"));
- goto done;
+ goto wait;
}
virt_viewer_app_show_status(app, _("Finding guest domain"));
@@ -649,9 +649,7 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
if (!dom) {
if (priv->waitvm) {
virt_viewer_app_show_status(app, _("Waiting for guest domain to be created"));
- virt_viewer_app_trace(app, "Guest %s does not yet exist, waiting for it to be created",
- priv->domkey);
- goto done;
+ goto wait;
} else {
dom = choose_vm(&priv->domkey, priv->conn, &err);
if (dom == NULL && err != NULL) {
@@ -675,27 +673,29 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
if (info.state == VIR_DOMAIN_SHUTOFF) {
virt_viewer_app_show_status(app, _("Waiting for guest domain to start"));
- } else {
- ret = virt_viewer_update_display(self, dom);
- if (ret)
- ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
- if (!ret) {
- if (priv->waitvm) {
- virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
- virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting for it to start",
- priv->domkey);
- } else {
- g_set_error_literal(&err, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
- _("Failed to activate viewer"));
- g_debug("%s", err->message);
- goto cleanup;
- }
+ goto wait;
+ }
+ ret = virt_viewer_update_display(self, dom);
+ if (ret)
+ ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
+ if (!ret) {
+ if (priv->waitvm) {
+ virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
+ goto wait;
+ } else {
+ g_set_error_literal(&err, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
+ _("Failed to activate viewer"));
+ g_debug("%s", err->message);
+ goto cleanup;
}
}
- done:
+wait:
+ virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting "
+ "for it to start", priv->domkey);
ret = TRUE;
- cleanup:
+
+cleanup:
if (err != NULL)
g_propagate_error(error, err);
if (dom)
--
2.1.0
From e54eccbc6eaef267ddf3fe29e74d3f04f04630ac Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@xxxxxxxxx>
Date: Thu, 13 Nov 2014 18:20:42 +0100
Subject: [virt-viewer 2/3] Simplify virt_viewer_initial_connect()
- remove need for waitvm if the display server isn't yet started (note:
this function might be untested, I am not sure relying on libvirt events
is enough)
---
src/virt-viewer.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/src/virt-viewer.c b/src/virt-viewer.c
index fe77d34..ef59c48 100644
--- a/src/virt-viewer.c
+++ b/src/virt-viewer.c
@@ -676,18 +676,11 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
goto wait;
}
ret = virt_viewer_update_display(self, dom);
- if (ret)
+ if (ret) {
ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
if (!ret) {
- if (priv->waitvm) {
- virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
- goto wait;
- } else {
- g_set_error_literal(&err, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
- _("Failed to activate viewer"));
- g_debug("%s", err->message);
- goto cleanup;
- }
+ virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
+ goto wait;
}
wait:
--
2.1.0
From 9825dee30d270e538b8f1af6632b08389c6f4697 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= <marcandre.lureau@xxxxxxxxx>
Date: Thu, 13 Nov 2014 18:20:42 +0100
Subject: [virt-viewer 3/3] Simplify virt_viewer_initial_connect()
- do not overwrite err if ->initial_connect() sets it
- remove need for waitvm if the display server isn't yet started (note:
this function might be untested, I am not sure relying on libvirt events
is enough)
---
src/virt-viewer.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/virt-viewer.c b/src/virt-viewer.c
index ef59c48..c1d2765 100644
--- a/src/virt-viewer.c
+++ b/src/virt-viewer.c
@@ -675,13 +675,13 @@ virt_viewer_initial_connect(VirtViewerApp *app, GError **error)
virt_viewer_app_show_status(app, _("Waiting for guest domain to start"));
goto wait;
}
- ret = virt_viewer_update_display(self, dom);
- if (ret) {
- ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
- if (!ret) {
- virt_viewer_app_show_status(app, _("Waiting for guest domain to start server"));
+
+ if (!virt_viewer_update_display(self, dom))
goto wait;
- }
+
+ ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app, &err);
+ if (ret || err)
+ goto cleanup;
wait:
virt_viewer_app_trace(app, "Guest %s has not activated its display yet, waiting "
--
2.1.0
Attachment:
pgpHhdltiOOwc.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
