| Message ID | 20230221232520.14480-6-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | bulk: Replace assert(0) -> g_assert_not_reached() | expand |
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);
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
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 >
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 > >
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~
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! */
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);
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(-)