| Message ID | 39dc6b92-dd56-fe33-cd85-92c03d69f133@redhat.com |
|---|---|
| State | New |
| Headers | show |
Hi, > FWIW, most of these have been fixed in the meantime; the only > remaining > hack I had to add was: > > diff --git i/hw/usb/bus.c w/hw/usb/bus.c > index 5939b273b9..bce011058b 100644 > --- i/hw/usb/bus.c > +++ w/hw/usb/bus.c > @@ -407,8 +407,9 @@ void usb_register_companion(const char > *masterbus, > USBPort *ports[], > void usb_port_location(USBPort *downstream, USBPort *upstream, int > portnr) > { > if (upstream) { > - snprintf(downstream->path, sizeof(downstream->path), > "%s.%d", > - upstream->path, portnr); > + int l = snprintf(downstream->path, sizeof(downstream->path), > "%s.%d", > + upstream->path, portnr); > + assert(l < sizeof(downstream->path)); Approach looks ok to me. Maximum hub chain length is 5, number of ports hubs is limited too, you'll never need more that two digits for port numbers. So 2*5 plus 4 connecting dots => 14 chars is the max strlen. path size is 16, so it will fit, including the terminating \0. Trying things like "assert(portnr <= 99)" have no effect on the possible string length calculated by gcc7. cheers, Gerd
On 07/17/2017 09:16 AM, Gerd Hoffmann wrote: > Hi, > >> FWIW, most of these have been fixed in the meantime; the only >> remaining >> hack I had to add was: >> >> diff --git i/hw/usb/bus.c w/hw/usb/bus.c >> index 5939b273b9..bce011058b 100644 >> --- i/hw/usb/bus.c >> +++ w/hw/usb/bus.c >> @@ -407,8 +407,9 @@ void usb_register_companion(const char >> *masterbus, >> USBPort *ports[], >> void usb_port_location(USBPort *downstream, USBPort *upstream, int >> portnr) >> { >> if (upstream) { >> - snprintf(downstream->path, sizeof(downstream->path), >> "%s.%d", >> - upstream->path, portnr); >> + int l = snprintf(downstream->path, sizeof(downstream->path), >> "%s.%d", >> + upstream->path, portnr); >> + assert(l < sizeof(downstream->path)); > > Approach looks ok to me. > > Maximum hub chain length is 5, number of ports hubs is limited too, > you'll never need more that two digits for port numbers. So 2*5 plus 4 > connecting dots => 14 chars is the max strlen. path size is 16, so it > will fit, including the terminating \0. Cool! With that text in the commit message and/or code comment, we can turn this from a hack into an actual patch, with no change in code. I'll go ahead and spin this as a formal patch. > > Trying things like "assert(portnr <= 99)" have no effect on the > possible string length calculated by gcc7. Perhaps a limitation that ought to be reported to the gcc folks.
* Eric Blake (eblake@redhat.com) wrote: > On 07/17/2017 08:42 AM, Eric Blake wrote: > > On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote: > >> On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote: > >>> Hi Dave, > >>> > >>> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote: > >>>> Hi, > >>>> Fedora 26 has gcc 7.0.1 which has the normal compliment > >>>> of new fussy warnings; so far I've posted : > >>>> > >>>> tests/check-qdict: Fix missing brackets > >>>> slirp/smb: Replace constant strings by glib string > >>>> > >>>> that fix one actual mistake and work around something it's being > >>>> fussy over. > >>>> > >>>> But I've also got a pile of hacks, attached below that I'm > >>>> not too sure what I'll do with them yet, but they're attached > >> > >> ping ... What do we do with them? > > > > Well, now that I've upgraded to the just-released Fedora 26, it is now > > mainline gcc and affecting my builds. So we should really try and find > > patches that silence the warnings (although it counts as bug fixes, so > > it won't hurt if it doesn't make tomorrow's freeze deadline). > > FWIW, most of these have been fixed in the meantime; the only remaining > hack I had to add was: > > diff --git i/hw/usb/bus.c w/hw/usb/bus.c > index 5939b273b9..bce011058b 100644 > --- i/hw/usb/bus.c > +++ w/hw/usb/bus.c > @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, > USBPort *ports[], > void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) > { > if (upstream) { > - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", > - upstream->path, portnr); > + int l = snprintf(downstream->path, sizeof(downstream->path), > "%s.%d", > + upstream->path, portnr); > + assert(l < sizeof(downstream->path)); You may find this doesn't help in some windows builds; the assert functions aren't always marked as noreturn (because they pop up a dialog that asks you whether you want to run into a debugger etc). Dave > downstream->hubcount = upstream->hubcount + 1; > } else { > snprintf(downstream->path, sizeof(downstream->path), "%d", portnr); > > Gerd, MAINTAINERS lists you; can you come up with something more robust? > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 17 July 2017 at 17:46, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote: > You may find this doesn't help in some windows builds; the assert > functions aren't always marked as noreturn (because they pop up a dialog > that asks you whether you want to run into a debugger etc). Oh, is that why? In any case, we rely on g_assert() and friends from glib being definitely marked noreturn, so we could use them instead. thanks -- PMM
On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: >> +++ w/hw/usb/bus.c >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, >> USBPort *ports[], >> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) >> { >> if (upstream) { >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", >> - upstream->path, portnr); >> + int l = snprintf(downstream->path, sizeof(downstream->path), >> "%s.%d", >> + upstream->path, portnr); >> + assert(l < sizeof(downstream->path)); > > You may find this doesn't help in some windows builds; the assert > functions aren't always marked as noreturn (because they pop up a dialog > that asks you whether you want to run into a debugger etc). How would it not help? Are we using gcc 7 on windows builds? Adding the assert is enough to shut up new gcc; old gcc was already silent; and if mingw is still on old gcc, it doesn't matter whether assert() is marked noreturn for what this patch is doing.
On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote: > On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: > >>> +++ w/hw/usb/bus.c >>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, >>> USBPort *ports[], >>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) >>> { >>> if (upstream) { >>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", >>> - upstream->path, portnr); >>> + int l = snprintf(downstream->path, sizeof(downstream->path), >>> "%s.%d", >>> + upstream->path, portnr); >>> + assert(l < sizeof(downstream->path)); >> >> You may find this doesn't help in some windows builds; the assert >> functions aren't always marked as noreturn (because they pop up a dialog >> that asks you whether you want to run into a debugger etc). > > How would it not help? Are we using gcc 7 on windows builds? At some point in the future we are likely to, because my w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64 packages, and so as I upgrade my desktop they will move forward to newer gcc versions. (More generally our users may do so before me ;-)) We should be consistent -- if we can't trust assert() to be marked nonreturn, as it seems we can't, then we shouldn't write new code that assumes it always is, even if today it doesn't happen to bite us on the compiler/host combinations we're testing right now. thanks -- PMM
* Eric Blake (eblake@redhat.com) wrote: > On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: > > >> +++ w/hw/usb/bus.c > >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, > >> USBPort *ports[], > >> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) > >> { > >> if (upstream) { > >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", > >> - upstream->path, portnr); > >> + int l = snprintf(downstream->path, sizeof(downstream->path), > >> "%s.%d", > >> + upstream->path, portnr); > >> + assert(l < sizeof(downstream->path)); > > > > You may find this doesn't help in some windows builds; the assert > > functions aren't always marked as noreturn (because they pop up a dialog > > that asks you whether you want to run into a debugger etc). > > How would it not help? Are we using gcc 7 on windows builds? Adding > the assert is enough to shut up new gcc; old gcc was already silent; and > if mingw is still on old gcc, it doesn't matter whether assert() is > marked noreturn for what this patch is doing. [dgilbert@dgilbert-t530 ~]$ x86_64-w64-mingw32-gcc -v Using built-in specs. COLLECT_GCC=/usr/bin/x86_64-w64-mingw32-gcc COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-w64-mingw32/7.1.0/lto-wrapper Target: x86_64-w64-mingw32 Configured with: ../configure --prefix=/usr --bindir=/usr/bin --includedir=/usr/include --mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --with-gnu-as --with-gnu-ld --verbose --without-newlib --disable-multilib --disable-plugin --with-system-zlib --disable-nls --without-included-gettext --disable-win32-registry --enable-languages=c,c++,objc,obj-c++,fortran --with-bugurl=http://bugzilla.redhat.com/bugzilla --with-cloog --enable-threads=posix --enable-libgomp --target=x86_64-w64-mingw32 --with-sysroot=/usr/x86_64-w64-mingw32/sys-root --with-gxx-include-dir=/usr/x86_64-w64-mingw32/sys-root/mingw/include/c++ Thread model: posix gcc version 7.1.0 20170502 (Fedora MinGW 7.1.0-1.fc26) (GCC) Dave > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 07/17/2017 12:36 PM, Peter Maydell wrote: > On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote: >> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: >> >>>> +++ w/hw/usb/bus.c >>>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, >>>> USBPort *ports[], >>>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) >>>> { >>>> if (upstream) { >>>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", >>>> - upstream->path, portnr); >>>> + int l = snprintf(downstream->path, sizeof(downstream->path), >>>> "%s.%d", >>>> + upstream->path, portnr); >>>> + assert(l < sizeof(downstream->path)); >>> >>> You may find this doesn't help in some windows builds; the assert >>> functions aren't always marked as noreturn (because they pop up a dialog >>> that asks you whether you want to run into a debugger etc). >> >> How would it not help? Are we using gcc 7 on windows builds? > > At some point in the future we are likely to, because my > w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64 > packages, and so as I upgrade my desktop they will move > forward to newer gcc versions. (More generally our users > may do so before me ;-)) So, does gcc's warning actually depend on the no-return-ness of the assert() statement added here? Would there be anything wrong with making osdep.h do this on mingw: #include <assert.h> #undef assert #define assert(expr) g_assert(expr) so that we can reliably get no-return handling that we desire, without having to audit lots of other code? > > We should be consistent -- if we can't trust assert() to > be marked nonreturn, as it seems we can't, then we shouldn't > write new code that assumes it always is, even if today > it doesn't happen to bite us on the compiler/host combinations > we're testing right now.
* Eric Blake (eblake@redhat.com) wrote: > On 07/17/2017 12:36 PM, Peter Maydell wrote: > > On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote: > >> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: > >> > >>>> +++ w/hw/usb/bus.c > >>>> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, > >>>> USBPort *ports[], > >>>> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) > >>>> { > >>>> if (upstream) { > >>>> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", > >>>> - upstream->path, portnr); > >>>> + int l = snprintf(downstream->path, sizeof(downstream->path), > >>>> "%s.%d", > >>>> + upstream->path, portnr); > >>>> + assert(l < sizeof(downstream->path)); > >>> > >>> You may find this doesn't help in some windows builds; the assert > >>> functions aren't always marked as noreturn (because they pop up a dialog > >>> that asks you whether you want to run into a debugger etc). > >> > >> How would it not help? Are we using gcc 7 on windows builds? > > > > At some point in the future we are likely to, because my > > w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64 > > packages, and so as I upgrade my desktop they will move > > forward to newer gcc versions. (More generally our users > > may do so before me ;-)) > > So, does gcc's warning actually depend on the no-return-ness of the > assert() statement added here? I'm not sure in this case, I'd hit it previously though, see my comment from 2015 in: https://sourceforge.net/p/mingw-w64/bugs/306/ > Would there be anything wrong with making osdep.h do this on mingw: > > #include <assert.h> > #undef assert > #define assert(expr) g_assert(expr) > > so that we can reliably get no-return handling that we desire, without > having to audit lots of other code? It's a bit nasty, but hmm. I only just about trust glib not to change, all of the fancier g_assert variants can return, but g_assert is still safe. Dave > > > > We should be consistent -- if we can't trust assert() to > > be marked nonreturn, as it seems we can't, then we shouldn't > > write new code that assumes it always is, even if today > > it doesn't happen to bite us on the compiler/host combinations > > we're testing right now. > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On Mon, Jul 17, 2017 at 12:29:08PM -0500, Eric Blake wrote: > On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: > > >> +++ w/hw/usb/bus.c > >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, > >> USBPort *ports[], > >> void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) > >> { > >> if (upstream) { > >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", > >> - upstream->path, portnr); > >> + int l = snprintf(downstream->path, sizeof(downstream->path), > >> "%s.%d", > >> + upstream->path, portnr); > >> + assert(l < sizeof(downstream->path)); > > > > You may find this doesn't help in some windows builds; the assert > > functions aren't always marked as noreturn (because they pop up a dialog > > that asks you whether you want to run into a debugger etc). > > How would it not help? Are we using gcc 7 on windows builds? Adding > the assert is enough to shut up new gcc; old gcc was already silent; and > if mingw is still on old gcc, it doesn't matter whether assert() is > marked noreturn for what this patch is doing. Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc is already on version 7.1.0 in Fedora Regards, Daniel
On 07/18/2017 02:23 AM, Daniel P. Berrange wrote: >> How would it not help? Are we using gcc 7 on windows builds? Adding >> the assert is enough to shut up new gcc; old gcc was already silent; and >> if mingw is still on old gcc, it doesn't matter whether assert() is >> marked noreturn for what this patch is doing. > > Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's > mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc > is already on version 7.1.0 in Fedora So I guess that means running 'make docker-test-mingw@fedora' on Fedora 26 will find out if we need further tweaks? Trying it now... Nope, didn't get very far :( $ make docker-test-mingw@fedora BUILD fedora make[1]: Entering directory '/home/eblake/qemu' ARCHIVE qemu.tgz usage: git archive [<options>] <tree-ish> [<path>...] ... make[1]: *** [/home/eblake/qemu/tests/docker/Makefile.include:35: docker-src.2017-07-18-07.35.20.4101] Error 129 make[1]: Leaving directory '/home/eblake/qemu' make: *** [/home/eblake/qemu/tests/docker/Makefile.include:156: docker-run-test-mingw@fedora] Error 2 Am I not doing it right?
On Tue, Jul 18, 2017 at 07:35:58AM -0500, Eric Blake wrote: > On 07/18/2017 02:23 AM, Daniel P. Berrange wrote: > >> How would it not help? Are we using gcc 7 on windows builds? Adding > >> the assert is enough to shut up new gcc; old gcc was already silent; and > >> if mingw is still on old gcc, it doesn't matter whether assert() is > >> marked noreturn for what this patch is doing. > > > > Mingw isn't using a fork of GCC anymore, its all mainline. Thus Fedora's > > mingw gcc packages track native gcc packages. IOW i686-w64-mingw32-gcc > > is already on version 7.1.0 in Fedora > > So I guess that means running 'make docker-test-mingw@fedora' on Fedora > 26 will find out if we need further tweaks? Trying it now... > > Nope, didn't get very far :( > > $ make docker-test-mingw@fedora > BUILD fedora > make[1]: Entering directory '/home/eblake/qemu' > ARCHIVE qemu.tgz > usage: git archive [<options>] <tree-ish> [<path>...] > ... > make[1]: *** [/home/eblake/qemu/tests/docker/Makefile.include:35: > docker-src.2017-07-18-07.35.20.4101] Error 129 > make[1]: Leaving directory '/home/eblake/qemu' > make: *** [/home/eblake/qemu/tests/docker/Makefile.include:156: > docker-run-test-mingw@fedora] Error 2 > > Am I not doing it right? That works fine for me - perhaps V=1 will tell you what's wrong with the git archive command. In any case, despite having gcc 7.1.0, I don't see the error messages in question when running a mingw build. It could be that the new problems GCC 7 reports are only triggered when combined with a suitable C library like GCC, to get the symbols annotated to enable fortify source to check them. Regards, Daniel
On 07/17/2017 02:36 PM, Peter Maydell wrote: > On 17 July 2017 at 18:29, Eric Blake <eblake@redhat.com> wrote: >> On 07/17/2017 11:46 AM, Dr. David Alan Gilbert wrote: [...] >>> You may find this doesn't help in some windows builds; the assert >>> functions aren't always marked as noreturn (because they pop up a dialog >>> that asks you whether you want to run into a debugger etc). >> >> How would it not help? Are we using gcc 7 on windows builds? > > At some point in the future we are likely to, because my > w32/w64 test setups use the Ubuntu gcc-mingw-w64-x86-64 > packages, and so as I upgrade my desktop they will move > forward to newer gcc versions. (More generally our users > may do so before me ;-)) The MXE toolchain (mxe.cc) provides GCC version 5.4.0 and compiles fine: win32: https://app.shippable.com/github/philmd/qemu/runs/32/2/console # file i386-softmmu/qemu-system-i386.exe i386-softmmu/qemu-system-i386.exe: PE32 executable (console) Intel 80386, for MS Windows and win64: https://app.shippable.com/github/philmd/qemu/runs/32/3/console # file x86_64-softmmu/qemu-system-x86_64.exe x86_64-softmmu/qemu-system-x86_64.exe: PE32+ executable (console) x86-64, for MS Windows > We should be consistent -- if we can't trust assert() to > be marked nonreturn, as it seems we can't, then we shouldn't > write new code that assumes it always is, even if today > it doesn't happen to bite us on the compiler/host combinations > we're testing right now. And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG some oldschool guys still have in their ~/.cshrc ;) Phil.
On 07/18/2017 10:04 AM, Philippe Mathieu-Daudé wrote: >> We should be consistent -- if we can't trust assert() to >> be marked nonreturn, as it seems we can't, then we shouldn't >> write new code that assumes it always is, even if today >> it doesn't happen to bite us on the compiler/host combinations >> we're testing right now. > > And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG > some oldschool guys still have in their ~/.cshrc ;) We don't have problems with people defining NDEBUG in their environment; such people would already hit at least: hw/scsi/mptsas.c:#ifdef NDEBUG hw/scsi/mptsas.c:#error building with NDEBUG is not supported (maybe we should hoist that to osdep.h, though)
On 07/17/2017 03:48 AM, Eric Blake wrote: > On 07/17/2017 08:42 AM, Eric Blake wrote: >> On 07/13/2017 08:07 AM, Philippe Mathieu-Daudé wrote: >>> On 04/07/2017 04:21 PM, Philippe Mathieu-Daudé wrote: >>>> Hi Dave, >>>> >>>> On 04/07/2017 11:38 AM, Dr. David Alan Gilbert wrote: >>>>> Hi, >>>>> Fedora 26 has gcc 7.0.1 which has the normal compliment >>>>> of new fussy warnings; so far I've posted : >>>>> >>>>> tests/check-qdict: Fix missing brackets >>>>> slirp/smb: Replace constant strings by glib string >>>>> >>>>> that fix one actual mistake and work around something it's being >>>>> fussy over. >>>>> >>>>> But I've also got a pile of hacks, attached below that I'm >>>>> not too sure what I'll do with them yet, but they're attached >>> >>> ping ... What do we do with them? >> >> Well, now that I've upgraded to the just-released Fedora 26, it is now >> mainline gcc and affecting my builds. So we should really try and find >> patches that silence the warnings (although it counts as bug fixes, so >> it won't hurt if it doesn't make tomorrow's freeze deadline). > > FWIW, most of these have been fixed in the meantime; the only remaining > hack I had to add was: > > diff --git i/hw/usb/bus.c w/hw/usb/bus.c > index 5939b273b9..bce011058b 100644 > --- i/hw/usb/bus.c > +++ w/hw/usb/bus.c > @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, > USBPort *ports[], > void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) > { > if (upstream) { > - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", > - upstream->path, portnr); > + int l = snprintf(downstream->path, sizeof(downstream->path), > "%s.%d", > + upstream->path, portnr); > + assert(l < sizeof(downstream->path)); Do you really need an assert there, or will (void)l; /* "used" */ work as well? You didn't mention what the reported error is, so I'm guessing. r~
On 07/18/2017 06:59 PM, Richard Henderson wrote: >> +++ w/hw/usb/bus.c >> @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, >> USBPort *ports[], >> void usb_port_location(USBPort *downstream, USBPort *upstream, int >> portnr) >> { >> if (upstream) { >> - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", >> - upstream->path, portnr); >> + int l = snprintf(downstream->path, sizeof(downstream->path), >> "%s.%d", >> + upstream->path, portnr); >> + assert(l < sizeof(downstream->path)); > > Do you really need an assert there, or will > > (void)l; /* "used" */ > > work as well? You didn't mention what the reported error is, so I'm > guessing. The original error is that gcc 7 complains that snprintf is prone to buffer overflow if the input is unbounded. Adding the assert that we KNOW the input is not unbounded is enough to shut up gcc, on Linux. What was then drawn into question is whether assert still has that property on mingw (since assert on mingw lacks the noreturn marking that it has on Linux). At this point, unless someone posts an actual failure of gcc 7 compiling this code for mingw, I don't see why we have to change it; shutting up the warning on Linux is good enough for the purpose of this patch.
On 18.07.2017 17:10, Eric Blake wrote: > On 07/18/2017 10:04 AM, Philippe Mathieu-Daudé wrote: >>> We should be consistent -- if we can't trust assert() to >>> be marked nonreturn, as it seems we can't, then we shouldn't >>> write new code that assumes it always is, even if today >>> it doesn't happen to bite us on the compiler/host combinations >>> we're testing right now. >> >> And there is also the problem when you compiles with CPPFLAGS+=-DNDEBUG >> some oldschool guys still have in their ~/.cshrc ;) > > We don't have problems with people defining NDEBUG in their environment; > such people would already hit at least: > > hw/scsi/mptsas.c:#ifdef NDEBUG > hw/scsi/mptsas.c:#error building with NDEBUG is not supported > > (maybe we should hoist that to osdep.h, though) Yes, please. Not every target is build with CONFIG_MPTSAS_SCSI_PCI so we should move that to a more central place. Thomas
diff --git i/hw/usb/bus.c w/hw/usb/bus.c index 5939b273b9..bce011058b 100644 --- i/hw/usb/bus.c +++ w/hw/usb/bus.c @@ -407,8 +407,9 @@ void usb_register_companion(const char *masterbus, USBPort *ports[], void usb_port_location(USBPort *downstream, USBPort *upstream, int portnr) { if (upstream) { - snprintf(downstream->path, sizeof(downstream->path), "%s.%d", - upstream->path, portnr); + int l = snprintf(downstream->path, sizeof(downstream->path), "%s.%d", + upstream->path, portnr); + assert(l < sizeof(downstream->path)); downstream->hubcount = upstream->hubcount + 1;