[virt-tools-list] [virt-viewer 1/2] app: monitor-config - do it all or nothing

[Fabiano Fidêncio]

On Mon, Mar 7, 2016 at 2:06 PM, Pavel Grunt <pgrunt at redhat.com> wrote:
> On Mon, 2016-03-07 at 13:16 +0100, Fabiano Fidêncio wrote:
>> On Mon, Mar 7, 2016 at 11:53 AM, Pavel Grunt <pgrunt at redhat.com>
>> wrote:
>> >
>> > On Mon, 2016-03-07 at 10:37 +0100, Fabiano Fidêncio wrote:
>> > >
>> > > Don't keep trying to use a monitor config when it already failed
>> > > for
>> > > one
>> > > monitor, otherwise virt-viewer can end up in a situation where
>> > > none
>> > > of
>> > > the displays are enabled but the program is still running.
>> > > So, in case of any failure, let's skip the whole monitor config,
>> > > forcing
>> > > virt-viewer to use the "fallback" one instead.
>> > >
>> > > Resolves: rhbz#1315206
>> > >
>> > > Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
>> > > ---
>> > >  src/virt-viewer-app.c | 7 ++++---
>> > >  1 file changed, 4 insertions(+), 3 deletions(-)
>> > >
>> > > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
>> > > index 660acef..bbb4928 100644
>> > > --- a/src/virt-viewer-app.c
>> > > +++ b/src/virt-viewer-app.c
>> > > @@ -404,9 +404,10 @@ virt_viewer_app_parse_monitor_mappings(gchar
>> > > **mappings, gsize nmappings)
>> > >          }
>> > >          g_strfreev(tokens);
>> > >
>> > > -        if (monitor > nmonitors)
>> > > -            g_warning("Initial monitor #%i for display #%i does
>> > > not
>> > > exist, skipping...", monitor, display);
>> > > -        else {
>> > > +        if (monitor > nmonitors) {
>> > > +            g_warning("Initial monitor #%i for display #%i does
>> > > not
>> > > exist", monitor, display);
>> > > +            goto configerror;
>> > > +        } else {
>> > >              /* config file format is 1-based, not 0-based */
>> > >              display--;
>> > >              monitor--;
>> > Hi, ack the change.
>> >
>> > It is considered as the error now, so I would use the same prefix
>> > as in
>> > any other monitor config message:
>> >
>> > "Invalid monitor-mapping configuration: the monitor #%i for display
>> > #%i
>> > does not exist"
>> Thanks for the suggestion.
>> I'm going to squash this patch[0] to the one you've acked.
>
> ack,
> Pavel
>
>>
>> [0]:
>> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
>> index 1cab1d9..f599a13 100644
>> --- a/src/virt-viewer-app.c
>> +++ b/src/virt-viewer-app.c
>> @@ -405,7 +405,7 @@ virt_viewer_app_parse_monitor_mappings(gchar
>> **mappings, gsize nmappings)
>>          g_strfreev(tokens);
>>
>>          if (monitor > nmonitors) {
>> -            g_warning("Initial monitor #%i for display #%i does not
>> exist", monitor, display);
>> +            g_warning("Invalid monitor-mapping configuration:
>> monitor
>> #%i for display #%i does not exist", monitor, display);
>>              goto configerror;
>>          }
>>
>>
>> Best Regards,
>> --
>> Fabiano Fidêncio

Pushed, thanks!