[virt-tools-list] [virt-viewer][PATCH 2/2] coverity: result is not floating-point

[Jonathon Jongsma]

On Tue, 2015-08-18 at 09:13 +0200, Pavel Grunt wrote:
> On Tue, 2015-08-18 at 01:41 +0200, Fabiano Fidencio wrote:
> > 
> > 
> > On Mon, Aug 17, 2015 at 6:08 PM, Pavel Grunt <pgrunt at redhat.com> wrote:
> > > On Mon, 2015-08-17 at 17:55 +0200, Fabiano Fidêncio wrote:
> > > > Coverity says:
> > > > Result is not floating-point (UNINTENDED_INTEGER_DIVISION)
> > > > interger_division: Dividing integer expressions "preferred->width * 100"
> > > > and "zoom", and then converting the integer quotient to type double. Any
> > > > remainder, or fractional part of the quotient, is ignored.
> > > 
> > > I think it is better to remove the round(), otherwise you are changing the
> > > behavior (which is there since 33614f86db490364339ef69e0eb76f98a4ac8138).
> 
> Hey Fabiano,
> I just suggested a way how to fix the coverity warning without changing the
> behavior.
> > I don't see why I am (wrongly) changing the behavior, Pavel.
> > Can you give me an example?
> 
> you gave the example, sometimes there will be an extra pixel, so I was afraid it
> can trigger some extra size allocation events. See
>  virt_viewer_display_size_allocate() in virt-viewer-display.c and 
> virt_viewer_display_spice_size_allocate() virt-viewer-display-spice.c and
> virt_viewer_session_on_monitor_geometry_changed() in virt-viewer-session.c
> 
> virt_viewer_session_on_monitor_geometry_changed() will be called if
>  virt_viewer_display_get_preferred_size() != GtkAllocation, and the requested
> size will be set using virt_viewer_display_get_preferred_monitor_geometry()
> 
> ACK if you test it and it is ok (no extra size request). It is also possible
> that you are avoiding the extra size request.


So I spent a little time testing and reading some related code. My main
concern was whether simply changing the zoom level would result in a
resize of the guest display. It turns out that this doesn't happen
because this function is not called in response to zoom changes (see the
early return in zoom_level_changed). So I think that the only time this
function is really called is after the window has been resized by the
user and we're about to send a new configuration down to the server. So
whether the value is rounded up or down at this point probably doesn't
make a lot of difference. I think it's probably best to use the more
"correct" approach. Additional testing would always be useful though.

Jonathon



> 
> > 
> > Nowadays, using the round or not using the 
> > round would end up in the same result and I don't think it's the 
> > expected/correct behavior.
> > 	
> > Let's consider: width = 640, NORMAL_ZOOM_LEVEL = 100, zoom = 85.
> > 
> > Current behavior:
> > width = round (640*100/85)
> > width = round (752)
> > width = 752.
> > 
> > Removing the round:
> > width = 640*100/85
> > width = 752
> > 
> > What I consider the expected behavior: 
> > width = round (640*100/85)
> > width = round (752.94)
> > width = 753
> > 
> >  
> > >  Or is the rounding necessary ?
> > I do think so.
> >  
> Thanks, it wasn't clear for me that it is about fixing some problem (is there a
> "black bar"? - it should be with your example).
>  
> Pavel
> 
> > >  
> > > Pavel
> > > > ---
> > > >  src/virt-viewer-display.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/virt-viewer-display.c b/src/virt-viewer-display.c
> > > > index 3efe24c..8431ae4 100644
> > > > --- a/src/virt-viewer-display.c
> > > > +++ b/src/virt-viewer-display.c
> > > > @@ -819,8 +819,8 @@ void
> > > > virt_viewer_display_get_preferred_monitor_geometry(VirtViewerDisplay* 
> > > self,
> > > >      if (virt_viewer_display_get_zoom(VIRT_VIEWER_DISPLAY(self))) {
> > > >          guint zoom =
> > > > virt_viewer_display_get_zoom_level(VIRT_VIEWER_DISPLAY(self));
> > > >
> > > > -        preferred->width = round(preferred->width * NORMAL_ZOOM_LEVEL /
> > > > zoom);
> > > > -        preferred->height = round(preferred->height * NORMAL_ZOOM_LEVEL /
> > > > zoom);
> > > > +        preferred->width = round(preferred->width * NORMAL_ZOOM_LEVEL /
> > > > (double) zoom);
> > > > +        preferred->height = round(preferred->height * NORMAL_ZOOM_LEVEL /
> > > > (double) zoom);
> > > >      }
> > > >  }
> > > >
> > > 
> > 
> > Thanks for the review,
> > -- 
> > Fabiano Fidêncio
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list