[virt-tools-list] [PATCH] Fix virt_viewer_app_activate return value

[Michal Privoznik]

On 14.06.2012 18:29, Christophe Fergeau wrote:
> Hey,
> 
> On Thu, Jun 14, 2012 at 05:06:06PM +0200, Michal Privoznik wrote:
>> The recent patch tried to fix return values
>> of this function. However, it resulted in swapped return values,
>> since if we were previously returning TRUE (1) we are now returning
>> FALSE (0). Fix this.
> 
> I'm actually a bit confused with virt_viewer_app_activate semantics,
> if you look at virt_viewer_app_activate , it returns -1 on error, but
> when you look at virt-viewer.c:515 (which you pointed me at), the code is:
> 
> 
> ret = virt_viewer_update_display(self, dom);
> if (ret >= 0)
>     ret = VIRT_VIEWER_APP_CLASS(virt_viewer_parent_class)->initial_connect(app);
> if (ret < 0) {
>     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\n",
>                               priv->domkey);
>     } else {
>         DEBUG_LOG("Failed to activate viewer");
>         goto cleanup;
>     }
> } else if (ret == 0) {
>     DEBUG_LOG("Failed to activate viewer");
>     ret = -1;
>     goto cleanup;
> }
> 
> (initial_connect is the same as activate in the VirtViewerApp class)
> 
> which handles both -1 and 0 as errors, so maybe the initial code was correct ?

It was indeed. That piece of code you are showing here is kind of messy
because it misuse ret variable a lot. Firstly,
virt_viewer_update_display returns a tri-state value, where anything
non-negative is TRUE, but initial_connect (which is
virt_viewer_app_default_activate in fact) returns a boolean value where
zero is suddenly FALSE.

So self-NAK as we need to cleanup this code in the first place.

Michal