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
> [email protected]
> 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?= <[email protected]> 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?= <[email protected]> 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?= <[email protected]> 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
pgp1VqoEE4MQV.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list [email protected] https://www.redhat.com/mailman/listinfo/virt-tools-list
