1. Patchew
  2. QEMU
  3. [PATCH 0/2] qxl: remove assert and migration blocker
  4. View patch

[PATCH 1/2] qxl: remove assert in qxl_pre_save.

Gerd Hoffmann posted 2 patches 4 years, 6 months ago
Maintainers: Gerd Hoffmann <kraxel@redhat.com>
[PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Gerd Hoffmann 4 years, 6 months ago 
Since commit 551dbd0846d2 ("migration: check pre_save return in
vmstate_save_state") the pre_save hook can fail.  So lets finally
use that to drop the guest-triggerable assert in qxl_pre_save().

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 84f99088e0a0..3867b94fe236 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
     } else {
         d->last_release_offset = (uint8_t *)d->last_release - ram_start;
     }
-    assert(d->last_release_offset < d->vga.vram_size);
+    if (d->last_release_offset < d->vga.vram_size) {
+        return 1;
+    }
 
     return 0;
 }
-- 
2.31.1
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Marc-André Lureau 4 years, 6 months ago 
On Wed, Jul 21, 2021 at 1:41 PM Gerd Hoffmann <kraxel@redhat.com> wrote:

> Since commit 551dbd0846d2 ("migration: check pre_save return in
> vmstate_save_state") the pre_save hook can fail.  So lets finally
> use that to drop the guest-triggerable assert in qxl_pre_save().
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

---
>  hw/display/qxl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 84f99088e0a0..3867b94fe236 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>      } else {
>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>      }
> -    assert(d->last_release_offset < d->vga.vram_size);
> +    if (d->last_release_offset < d->vga.vram_size) {
> +        return 1;
> +    }
>
>      return 0;
>  }
> --
> 2.31.1
>
>
>

-- 
Marc-André Lureau
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Dr. David Alan Gilbert 4 years, 6 months ago 
* Gerd Hoffmann (kraxel@redhat.com) wrote:
> Since commit 551dbd0846d2 ("migration: check pre_save return in
> vmstate_save_state") the pre_save hook can fail.  So lets finally
> use that to drop the guest-triggerable assert in qxl_pre_save().
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 84f99088e0a0..3867b94fe236 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>      } else {
>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>      }
> -    assert(d->last_release_offset < d->vga.vram_size);
> +    if (d->last_release_offset < d->vga.vram_size) {
> +        return 1;

It would be great if there was an error_report or something there
so that we get some idea of what happened.

Dave

> +    }
>  
>      return 0;
>  }
> -- 
> 2.31.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Gerd Hoffmann 4 years, 6 months ago 
On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
> * Gerd Hoffmann (kraxel@redhat.com) wrote:
> > Since commit 551dbd0846d2 ("migration: check pre_save return in
> > vmstate_save_state") the pre_save hook can fail.  So lets finally
> > use that to drop the guest-triggerable assert in qxl_pre_save().
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> > ---
> >  hw/display/qxl.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index 84f99088e0a0..3867b94fe236 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
> >      } else {
> >          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
> >      }
> > -    assert(d->last_release_offset < d->vga.vram_size);
> > +    if (d->last_release_offset < d->vga.vram_size) {
> > +        return 1;
> 
> It would be great if there was an error_report or something there
> so that we get some idea of what happened.

Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
the correct approach here, but I guess that wouldn't help much for
trouble-shooting as the message would only show up with "-d
guest_errors".  So better make that error_report("...") ?

take care,
  Gerd
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago 
On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>
>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>> ---
>>>  hw/display/qxl.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>> index 84f99088e0a0..3867b94fe236 100644
>>> --- a/hw/display/qxl.c
>>> +++ b/hw/display/qxl.c
>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>      } else {
>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>      }
>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>> +        return 1;
>>
>> It would be great if there was an error_report or something there
>> so that we get some idea of what happened.
> 
> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
> the correct approach here, but I guess that wouldn't help much for
> trouble-shooting as the message would only show up with "-d
> guest_errors".  So better make that error_report("...") ?

It depends who is interested in such errors. Does management app care?
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Gerd Hoffmann 4 years, 6 months ago 
On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
> > On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
> >> * Gerd Hoffmann (kraxel@redhat.com) wrote:
> >>> Since commit 551dbd0846d2 ("migration: check pre_save return in
> >>> vmstate_save_state") the pre_save hook can fail.  So lets finally
> >>> use that to drop the guest-triggerable assert in qxl_pre_save().
> >>>
> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >>> ---
> >>>  hw/display/qxl.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> >>> index 84f99088e0a0..3867b94fe236 100644
> >>> --- a/hw/display/qxl.c
> >>> +++ b/hw/display/qxl.c
> >>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
> >>>      } else {
> >>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
> >>>      }
> >>> -    assert(d->last_release_offset < d->vga.vram_size);
> >>> +    if (d->last_release_offset < d->vga.vram_size) {
> >>> +        return 1;
> >>
> >> It would be great if there was an error_report or something there
> >> so that we get some idea of what happened.
> > 
> > Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
> > the correct approach here, but I guess that wouldn't help much for
> > trouble-shooting as the message would only show up with "-d
> > guest_errors".  So better make that error_report("...") ?
> 
> It depends who is interested in such errors.

Migration fails and you want know why.

Reason migration fails is the guest violates qxl specs (which doesn't
cause actual problems unless you want migrate the guest).

take care,
  Gerd
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Philippe Mathieu-Daudé 4 years, 6 months ago 
On 7/23/21 8:55 AM, Gerd Hoffmann wrote:
> On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
>> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
>>> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>>>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>>>
>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>> ---
>>>>>  hw/display/qxl.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>>>> index 84f99088e0a0..3867b94fe236 100644
>>>>> --- a/hw/display/qxl.c
>>>>> +++ b/hw/display/qxl.c
>>>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>>>      } else {
>>>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>>>      }
>>>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>>>> +        return 1;
>>>>
>>>> It would be great if there was an error_report or something there
>>>> so that we get some idea of what happened.
>>>
>>> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
>>> the correct approach here, but I guess that wouldn't help much for
>>> trouble-shooting as the message would only show up with "-d
>>> guest_errors".  So better make that error_report("...") ?
>>
>> It depends who is interested in such errors.
> 
> Migration fails and you want know why.
> 
> Reason migration fails is the guest violates qxl specs (which doesn't
> cause actual problems unless you want migrate the guest).

Then I agree error_report() is better ("Report an error to the current
monitor if we have one, else stderr").
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Markus Armbruster 4 years, 6 months ago 
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/23/21 8:55 AM, Gerd Hoffmann wrote:
>> On Fri, Jul 23, 2021 at 08:47:09AM +0200, Philippe Mathieu-Daudé wrote:
>>> On 7/23/21 8:32 AM, Gerd Hoffmann wrote:
>>>> On Thu, Jul 22, 2021 at 03:44:54PM +0100, Dr. David Alan Gilbert wrote:
>>>>> * Gerd Hoffmann (kraxel@redhat.com) wrote:
>>>>>> Since commit 551dbd0846d2 ("migration: check pre_save return in
>>>>>> vmstate_save_state") the pre_save hook can fail.  So lets finally
>>>>>> use that to drop the guest-triggerable assert in qxl_pre_save().
>>>>>>
>>>>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>>>>> ---
>>>>>>  hw/display/qxl.c | 4 +++-
>>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>>>>>> index 84f99088e0a0..3867b94fe236 100644
>>>>>> --- a/hw/display/qxl.c
>>>>>> +++ b/hw/display/qxl.c
>>>>>> @@ -2283,7 +2283,9 @@ static int qxl_pre_save(void *opaque)
>>>>>>      } else {
>>>>>>          d->last_release_offset = (uint8_t *)d->last_release - ram_start;
>>>>>>      }
>>>>>> -    assert(d->last_release_offset < d->vga.vram_size);
>>>>>> +    if (d->last_release_offset < d->vga.vram_size) {
>>>>>> +        return 1;
>>>>>
>>>>> It would be great if there was an error_report or something there
>>>>> so that we get some idea of what happened.
>>>>
>>>> Hmm.  Strictly speaking qemu_log_mask(LOG_GUEST_ERROR, "..."); would be
>>>> the correct approach here, but I guess that wouldn't help much for
>>>> trouble-shooting as the message would only show up with "-d
>>>> guest_errors".  So better make that error_report("...") ?
>>>
>>> It depends who is interested in such errors.
>> 
>> Migration fails and you want know why.
>> 
>> Reason migration fails is the guest violates qxl specs (which doesn't
>> cause actual problems unless you want migrate the guest).
>
> Then I agree error_report() is better ("Report an error to the current
> monitor if we have one, else stderr").

No good for QMP, I'm afraid.

error_report() reports to stderr in QMP monitor context.  To get an
error to the QMP client, you need to use error_setg() and propagate all
the way to the QMP core.

Back to the patch.  If the thing we report is actually an *error*,
i.e. something that makes migration fail, then the QMP failure will
contain an error message.  That error must come from further up the call
stack.  If it's good enough, we don't need error_report() here.  Else,
we should error_setg() here.

If it's not actually an error, then error_report() is wrong.
warn_report() might be right.
Re: [PATCH 1/2] qxl: remove assert in qxl_pre_save.
Posted by Gerd Hoffmann 4 years, 6 months ago 
  Hi,

> >> Migration fails and you want know why.
> >> 
> >> Reason migration fails is the guest violates qxl specs (which doesn't
> >> cause actual problems unless you want migrate the guest).
> >
> > Then I agree error_report() is better ("Report an error to the current
> > monitor if we have one, else stderr").
> 
> No good for QMP, I'm afraid.
> 
> error_report() reports to stderr in QMP monitor context.  To get an
> error to the QMP client, you need to use error_setg() and propagate all
> the way to the QMP core.
> 
> Back to the patch.  If the thing we report is actually an *error*,
> i.e. something that makes migration fail, then the QMP failure will
> contain an error message.  That error must come from further up the call
> stack.  If it's good enough, we don't need error_report() here.  Else,
> we should error_setg() here.

Caller is here (migration/vmstate.c):

int vmstate_save_state_v(QEMUFile *f, const VMStateDescription *vmsd,
                         void *opaque, JSONWriter *vmdesc, int version_id)
{
    [ ... ]
    if (vmsd->pre_save) {
        ret = vmsd->pre_save(opaque);
        trace_vmstate_save_state_pre_save_res(vmsd->name, ret);
        if (ret) {
            error_report("pre-save failed: %s", vmsd->name);
            return ret;
        }
    }
    [ ... ]
}

No error_setg() anywhere close, it's error_report() all over the place.
I guess that makes adding error_report() the only short-term (aka 6.1)
option.

take care,
  Gerd

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.