bulk: Replace assert(0) -> g_assert_not_reached()

[PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

Since commit 262a69f428 ("osdep.h: Prohibit disabling
assert() in supported builds") 'NDEBUG' can not be defined.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/scsi/mptsas.c   | 2 --
 hw/virtio/virtio.c | 2 --
 2 files changed, 4 deletions(-)

diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index c485da792c..5b373d3ed6 100644
--- a/hw/scsi/mptsas.c
+++ b/hw/scsi/mptsas.c
@@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
     n = qemu_get_be32(f);
     /* TODO: add a way for SCSIBusInfo's load_request to fail,
      * and fail migration instead of asserting here.
-     * This is just one thing (there are probably more) that must be
-     * fixed before we can allow NDEBUG compilation.
      */
     assert(n >= 0);
 
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..c6b3e3fb08 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
 
     /* TODO: teach all callers that this can fail, and return failure instead
      * of asserting here.
-     * This is just one thing (there are probably more) that must be
-     * fixed before we can allow NDEBUG compilation.
      */
     assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
     assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
-- 
2.38.1

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Michael S. Tsirkin 2 years, 11 months ago

On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> Since commit 262a69f428 ("osdep.h: Prohibit disabling
> assert() in supported builds") 'NDEBUG' can not be defined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

this exactly says NDEBUG is not allowed. why are you removing this?
  
> ---
>  hw/scsi/mptsas.c   | 2 --
>  hw/virtio/virtio.c | 2 --
>  2 files changed, 4 deletions(-)
> 
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index c485da792c..5b373d3ed6 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>      n = qemu_get_be32(f);
>      /* TODO: add a way for SCSIBusInfo's load_request to fail,
>       * and fail migration instead of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>       */
>      assert(n >= 0);
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..c6b3e3fb08 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>  
>      /* TODO: teach all callers that this can fail, and return failure instead
>       * of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>       */
>      assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>      assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
> -- 
> 2.38.1

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Philippe Mathieu-Daudé 2 years, 11 months ago

On 22/2/23 13:05, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
>> Since commit 262a69f428 ("osdep.h: Prohibit disabling
>> assert() in supported builds") 'NDEBUG' can not be defined.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> this exactly says NDEBUG is not allowed. why are you removing this?

The project can not be built with NDEBUG. There is no point in
mentioning it in each individual function.

>> ---
>>   hw/scsi/mptsas.c   | 2 --
>>   hw/virtio/virtio.c | 2 --
>>   2 files changed, 4 deletions(-)
>>
>> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
>> index c485da792c..5b373d3ed6 100644
>> --- a/hw/scsi/mptsas.c
>> +++ b/hw/scsi/mptsas.c
>> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>>       n = qemu_get_be32(f);
>>       /* TODO: add a way for SCSIBusInfo's load_request to fail,
>>        * and fail migration instead of asserting here.
>> -     * This is just one thing (there are probably more) that must be
>> -     * fixed before we can allow NDEBUG compilation.
>>        */
>>       assert(n >= 0);
>>
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index f35178f5fc..c6b3e3fb08 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>>   
>>       /* TODO: teach all callers that this can fail, and return failure instead
>>        * of asserting here.
>> -     * This is just one thing (there are probably more) that must be
>> -     * fixed before we can allow NDEBUG compilation.
>>        */
>>       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>>       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
>> -- 
>> 2.38.1
>

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Michael S. Tsirkin 2 years, 11 months ago

On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
> On 22/2/23 13:05, Michael S. Tsirkin wrote:
> > On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> > > Since commit 262a69f428 ("osdep.h: Prohibit disabling
> > > assert() in supported builds") 'NDEBUG' can not be defined.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > this exactly says NDEBUG is not allowed. why are you removing this?
> 
> The project can not be built with NDEBUG. There is no point in
> mentioning it in each individual function.

the reason we mention it is because there are security implications
if we don't.

> > > ---
> > >   hw/scsi/mptsas.c   | 2 --
> > >   hw/virtio/virtio.c | 2 --
> > >   2 files changed, 4 deletions(-)
> > > 
> > > diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> > > index c485da792c..5b373d3ed6 100644
> > > --- a/hw/scsi/mptsas.c
> > > +++ b/hw/scsi/mptsas.c
> > > @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
> > >       n = qemu_get_be32(f);
> > >       /* TODO: add a way for SCSIBusInfo's load_request to fail,
> > >        * and fail migration instead of asserting here.
> > > -     * This is just one thing (there are probably more) that must be
> > > -     * fixed before we can allow NDEBUG compilation.
> > >        */
> > >       assert(n >= 0);
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index f35178f5fc..c6b3e3fb08 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
> > >       /* TODO: teach all callers that this can fail, and return failure instead
> > >        * of asserting here.
> > > -     * This is just one thing (there are probably more) that must be
> > > -     * fixed before we can allow NDEBUG compilation.
> > >        */
> > >       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
> > >       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);
> > > -- 
> > > 2.38.1
> >

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Richard Henderson 2 years, 11 months ago

On 2/22/23 06:28, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
>> On 22/2/23 13:05, Michael S. Tsirkin wrote:
>>> On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
>>>> Since commit 262a69f428 ("osdep.h: Prohibit disabling
>>>> assert() in supported builds") 'NDEBUG' can not be defined.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>
>>> this exactly says NDEBUG is not allowed. why are you removing this?
>>
>> The project can not be built with NDEBUG. There is no point in
>> mentioning it in each individual function.
> 
> the reason we mention it is because there are security implications
> if we don't.

Yes.  However that's not what the text being removed suggests:

>>>> -     * This is just one thing (there are probably more) that must be
>>>> -     * fixed before we can allow NDEBUG compilation.

This suggests that we *will* allow NDEBUG, once a few things are fixed.

I strongly approve of this text being removed.


r~

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Michael S. Tsirkin 2 years, 11 months ago

On Wed, Feb 22, 2023 at 08:43:35AM -1000, Richard Henderson wrote:
> On 2/22/23 06:28, Michael S. Tsirkin wrote:
> > On Wed, Feb 22, 2023 at 05:11:36PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 22/2/23 13:05, Michael S. Tsirkin wrote:
> > > > On Wed, Feb 22, 2023 at 12:25:20AM +0100, Philippe Mathieu-Daudé wrote:
> > > > > Since commit 262a69f428 ("osdep.h: Prohibit disabling
> > > > > assert() in supported builds") 'NDEBUG' can not be defined.
> > > > > 
> > > > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > > 
> > > > this exactly says NDEBUG is not allowed. why are you removing this?
> > > 
> > > The project can not be built with NDEBUG. There is no point in
> > > mentioning it in each individual function.
> > 
> > the reason we mention it is because there are security implications
> > if we don't.
> 
> Yes.  However that's not what the text being removed suggests:
> 
> > > > > -     * This is just one thing (there are probably more) that must be
> > > > > -     * fixed before we can allow NDEBUG compilation.
> 
> This suggests that we *will* allow NDEBUG, once a few things are fixed.
> 
> I strongly approve of this text being removed.
> 
> 
> r~


OK I think it's a good idea to replace it with something like

/* Note: Do not remove this assertion, doing so will break qemu security! */

-- 
MST

Re: [PATCH 5/5] hw: Remove mentions of NDEBUG

Posted by Richard Henderson 2 years, 11 months ago

On 2/21/23 13:25, Philippe Mathieu-Daudé wrote:
> Since commit 262a69f428 ("osdep.h: Prohibit disabling
> assert() in supported builds") 'NDEBUG' can not be defined.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/scsi/mptsas.c   | 2 --
>   hw/virtio/virtio.c | 2 --
>   2 files changed, 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

> 
> diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
> index c485da792c..5b373d3ed6 100644
> --- a/hw/scsi/mptsas.c
> +++ b/hw/scsi/mptsas.c
> @@ -1240,8 +1240,6 @@ static void *mptsas_load_request(QEMUFile *f, SCSIRequest *sreq)
>       n = qemu_get_be32(f);
>       /* TODO: add a way for SCSIBusInfo's load_request to fail,
>        * and fail migration instead of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>        */
>       assert(n >= 0);
>   
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f35178f5fc..c6b3e3fb08 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1898,8 +1898,6 @@ void *qemu_get_virtqueue_element(VirtIODevice *vdev, QEMUFile *f, size_t sz)
>   
>       /* TODO: teach all callers that this can fail, and return failure instead
>        * of asserting here.
> -     * This is just one thing (there are probably more) that must be
> -     * fixed before we can allow NDEBUG compilation.
>        */
>       assert(ARRAY_SIZE(data.in_addr) >= data.in_num);
>       assert(ARRAY_SIZE(data.out_addr) >= data.out_num);