| Message ID | 1449305702-2122-3-git-send-email-yanmiaobest@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 12/05/2015 04:55 PM, Miao Yan wrote: > Vmxnet3 uses the following debug macro style: > > #ifdef SOME_DEBUG > # define debug(...) do{ printf(...); } while (0) > # else > # define debug(...) do{ } while (0) > #endif > > If SOME_DEBUG is undefined, then format string inside the > debug macro will never be checked by compiler. Code is > likely to break in the future when SOME_DEBUG is enabled > because of lack of testing. This patch changes this > to the following: > > #define debug(...) \ > do { if (SOME_DEBUG_ENABLED) printf(...); } while (0) > > Signed-off-by: Miao Yan <yanmiaobest@gmail.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > --- > Changes in v2: > - Fix grammar error in commit log Applied in my -net for 2.5. Thanks > > hw/net/vmxnet_debug.h | 139 +++++++++++++++++++++++++++++++------------------- > 1 file changed, 86 insertions(+), 53 deletions(-) > > diff --git a/hw/net/vmxnet_debug.h b/hw/net/vmxnet_debug.h > index 96dae0f..96495db 100644 > --- a/hw/net/vmxnet_debug.h > +++ b/hw/net/vmxnet_debug.h > @@ -20,94 +20,127 @@ > > #define VMXNET_DEVICE_NAME "vmxnet3" > > -/* #define VMXNET_DEBUG_CB */ > #define VMXNET_DEBUG_WARNINGS > #define VMXNET_DEBUG_ERRORS > -/* #define VMXNET_DEBUG_INTERRUPTS */ > -/* #define VMXNET_DEBUG_CONFIG */ > -/* #define VMXNET_DEBUG_RINGS */ > -/* #define VMXNET_DEBUG_PACKETS */ > -/* #define VMXNET_DEBUG_SHMEM_ACCESS */ > + > +#undef VMXNET_DEBUG_CB > +#undef VMXNET_DEBUG_INTERRUPTS > +#undef VMXNET_DEBUG_CONFIG > +#undef VMXNET_DEBUG_RINGS > +#undef VMXNET_DEBUG_PACKETS > +#undef VMXNET_DEBUG_SHMEM_ACCESS > + > +#ifdef VMXNET_DEBUG_CB > +# define VMXNET_DEBUG_CB_ENABLED 1 > +#else > +# define VMXNET_DEBUG_CB_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_WARNINGS > +# define VMXNET_DEBUG_WARNINGS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_WARNINGS_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_ERRORS > +# define VMXNET_DEBUG_ERRORS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_ERRORS_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_CONFIG > +# define VMXNET_DEBUG_CONFIG_ENABLED 1 > +#else > +# define VMXNET_DEBUG_CONFIG_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_RINGS > +# define VMXNET_DEBUG_RINGS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_RINGS_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_PACKETS > +# define VMXNET_DEBUG_PACKETS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_PACKETS_ENABLED 0 > +#endif > + > +#ifdef VMXNET_DEBUG_INTERRUPTS > +# define VMXNET_DEBUG_INTERRUPTS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_INTERRUPTS_ENABLED 0 > +#endif > > #ifdef VMXNET_DEBUG_SHMEM_ACCESS > +# define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 1 > +#else > +# define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 0 > +#endif > + > #define VMW_SHPRN(fmt, ...) \ > do { \ > - printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_SHMEM_ACCESS_ENABLED) { \ > + printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_SHPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_CB > #define VMW_CBPRN(fmt, ...) \ > do { \ > - printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_CB_ENABLED) { \ > + printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_CBPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_PACKETS > #define VMW_PKPRN(fmt, ...) \ > do { \ > - printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_PACKETS_ENABLED) { \ > + printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_PKPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_WARNINGS > #define VMW_WRPRN(fmt, ...) \ > do { \ > - printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_WARNINGS_ENABLED) { \ > + printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_WRPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_ERRORS > #define VMW_ERPRN(fmt, ...) \ > do { \ > - printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_ERRORS_ENABLED) { \ > + printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_ERPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_INTERRUPTS > #define VMW_IRPRN(fmt, ...) \ > do { \ > - printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_INTERRUPTS_ENABLED) { \ > + printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_IRPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_CONFIG > #define VMW_CFPRN(fmt, ...) \ > do { \ > - printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_CONFIG_ENABLED) { \ > + printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_CFPRN(fmt, ...) do {} while (0) > -#endif > > -#ifdef VMXNET_DEBUG_RINGS > #define VMW_RIPRN(fmt, ...) \ > do { \ > - printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > - ## __VA_ARGS__); \ > + if (VMXNET_DEBUG_RINGS_ENABLED) { \ > + printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ > + ## __VA_ARGS__); \ > + } \ > } while (0) > -#else > -#define VMW_RIPRN(fmt, ...) do {} while (0) > -#endif > > #define VMXNET_MF "%02X:%02X:%02X:%02X:%02X:%02X" > #define VMXNET_MA(a) (a)[0], (a)[1], (a)[2], (a)[3], (a)[4], (a)[5]
On 12/06/2015 07:47 PM, Jason Wang wrote: > > > On 12/05/2015 04:55 PM, Miao Yan wrote: >> Vmxnet3 uses the following debug macro style: >> >> #ifdef SOME_DEBUG >> # define debug(...) do{ printf(...); } while (0) >> # else >> # define debug(...) do{ } while (0) >> #endif >> >> If SOME_DEBUG is undefined, then format string inside the >> debug macro will never be checked by compiler. Code is >> likely to break in the future when SOME_DEBUG is enabled >> because of lack of testing. This patch changes this >> to the following: >> >> #define debug(...) \ >> do { if (SOME_DEBUG_ENABLED) printf(...); } while (0) >> >> Signed-off-by: Miao Yan <yanmiaobest@gmail.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> --- >> Changes in v2: >> - Fix grammar error in commit log > > Applied in my -net for 2.5. Thanks I don't know if Miao saw Peter's reaction to the pull request, but just as I guessed on v1, exposing more format strings turned up more latent bugs in the format strings, with failures such as: > I'm afraid this doesn't build on 32-bit due to format string errors: > > /home/petmay01/qemu/hw/net/vmxnet3.c: In function 'vmxnet3_complete_packet': > /home/petmay01/qemu/hw/net/vmxnet3.c:500:5: error: format '%lu' > expects argument of type 'long unsigned int', but argument 7 has type > 'size_t' [-Werror=format=] > VMXNET3_RING_DUMP(VMW_RIPRN, "TXC", qidx, &s->txq_descr[qidx].comp_ring); Looks like it will need a v3; and sorry that I didn't catch the problems in my review (I was just going off of whether the macro conversion was correct, and not an actual compile test to see if all the now-live format strings were always valid).
2015-12-08 6:04 GMT+08:00 Eric Blake <eblake@redhat.com>: > On 12/06/2015 07:47 PM, Jason Wang wrote: >> >> >> On 12/05/2015 04:55 PM, Miao Yan wrote: >>> Vmxnet3 uses the following debug macro style: >>> >>> #ifdef SOME_DEBUG >>> # define debug(...) do{ printf(...); } while (0) >>> # else >>> # define debug(...) do{ } while (0) >>> #endif >>> >>> If SOME_DEBUG is undefined, then format string inside the >>> debug macro will never be checked by compiler. Code is >>> likely to break in the future when SOME_DEBUG is enabled >>> because of lack of testing. This patch changes this >>> to the following: >>> >>> #define debug(...) \ >>> do { if (SOME_DEBUG_ENABLED) printf(...); } while (0) >>> >>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> --- >>> Changes in v2: >>> - Fix grammar error in commit log >> >> Applied in my -net for 2.5. Thanks > > I don't know if Miao saw Peter's reaction to the pull request, but just > as I guessed on v1, exposing more format strings turned up more latent > bugs in the format strings, with failures such as: Very sorry about this. > >> I'm afraid this doesn't build on 32-bit due to format string errors: >> >> /home/petmay01/qemu/hw/net/vmxnet3.c: In function 'vmxnet3_complete_packet': >> /home/petmay01/qemu/hw/net/vmxnet3.c:500:5: error: format '%lu' >> expects argument of type 'long unsigned int', but argument 7 has type >> 'size_t' [-Werror=format=] >> VMXNET3_RING_DUMP(VMW_RIPRN, "TXC", qidx, &s->txq_descr[qidx].comp_ring); I don't have a 32 bit machine for a long time, I'll create a VM to test this. > > Looks like it will need a v3; and sorry that I didn't catch the problems > in my review (I was just going off of whether the macro conversion was > correct, and not an actual compile test to see if all the now-live > format strings were always valid). I guess %zu should fix this. I'll send v3. Thanks. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On 12/08/2015 06:04 AM, Eric Blake wrote: > On 12/06/2015 07:47 PM, Jason Wang wrote: >> >> On 12/05/2015 04:55 PM, Miao Yan wrote: >>> Vmxnet3 uses the following debug macro style: >>> >>> #ifdef SOME_DEBUG >>> # define debug(...) do{ printf(...); } while (0) >>> # else >>> # define debug(...) do{ } while (0) >>> #endif >>> >>> If SOME_DEBUG is undefined, then format string inside the >>> debug macro will never be checked by compiler. Code is >>> likely to break in the future when SOME_DEBUG is enabled >>> because of lack of testing. This patch changes this >>> to the following: >>> >>> #define debug(...) \ >>> do { if (SOME_DEBUG_ENABLED) printf(...); } while (0) >>> >>> Signed-off-by: Miao Yan <yanmiaobest@gmail.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> --- >>> Changes in v2: >>> - Fix grammar error in commit log >> Applied in my -net for 2.5. Thanks > I don't know if Miao saw Peter's reaction to the pull request, but just > as I guessed on v1, exposing more format strings turned up more latent > bugs in the format strings, with failures such as: > >> I'm afraid this doesn't build on 32-bit due to format string errors: >> >> /home/petmay01/qemu/hw/net/vmxnet3.c: In function 'vmxnet3_complete_packet': >> /home/petmay01/qemu/hw/net/vmxnet3.c:500:5: error: format '%lu' >> expects argument of type 'long unsigned int', but argument 7 has type >> 'size_t' [-Werror=format=] >> VMXNET3_RING_DUMP(VMW_RIPRN, "TXC", qidx, &s->txq_descr[qidx].comp_ring); > Looks like it will need a v3; and sorry that I didn't catch the problems > in my review (I was just going off of whether the macro conversion was > correct, and not an actual compile test to see if all the now-live > format strings were always valid). > No problem, this also reminds me to do 32-bit compile test before submitting pull request. Thanks
diff --git a/hw/net/vmxnet_debug.h b/hw/net/vmxnet_debug.h index 96dae0f..96495db 100644 --- a/hw/net/vmxnet_debug.h +++ b/hw/net/vmxnet_debug.h @@ -20,94 +20,127 @@ #define VMXNET_DEVICE_NAME "vmxnet3" -/* #define VMXNET_DEBUG_CB */ #define VMXNET_DEBUG_WARNINGS #define VMXNET_DEBUG_ERRORS -/* #define VMXNET_DEBUG_INTERRUPTS */ -/* #define VMXNET_DEBUG_CONFIG */ -/* #define VMXNET_DEBUG_RINGS */ -/* #define VMXNET_DEBUG_PACKETS */ -/* #define VMXNET_DEBUG_SHMEM_ACCESS */ + +#undef VMXNET_DEBUG_CB +#undef VMXNET_DEBUG_INTERRUPTS +#undef VMXNET_DEBUG_CONFIG +#undef VMXNET_DEBUG_RINGS +#undef VMXNET_DEBUG_PACKETS +#undef VMXNET_DEBUG_SHMEM_ACCESS + +#ifdef VMXNET_DEBUG_CB +# define VMXNET_DEBUG_CB_ENABLED 1 +#else +# define VMXNET_DEBUG_CB_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_WARNINGS +# define VMXNET_DEBUG_WARNINGS_ENABLED 1 +#else +# define VMXNET_DEBUG_WARNINGS_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_ERRORS +# define VMXNET_DEBUG_ERRORS_ENABLED 1 +#else +# define VMXNET_DEBUG_ERRORS_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_CONFIG +# define VMXNET_DEBUG_CONFIG_ENABLED 1 +#else +# define VMXNET_DEBUG_CONFIG_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_RINGS +# define VMXNET_DEBUG_RINGS_ENABLED 1 +#else +# define VMXNET_DEBUG_RINGS_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_PACKETS +# define VMXNET_DEBUG_PACKETS_ENABLED 1 +#else +# define VMXNET_DEBUG_PACKETS_ENABLED 0 +#endif + +#ifdef VMXNET_DEBUG_INTERRUPTS +# define VMXNET_DEBUG_INTERRUPTS_ENABLED 1 +#else +# define VMXNET_DEBUG_INTERRUPTS_ENABLED 0 +#endif #ifdef VMXNET_DEBUG_SHMEM_ACCESS +# define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 1 +#else +# define VMXNET_DEBUG_SHMEM_ACCESS_ENABLED 0 +#endif + #define VMW_SHPRN(fmt, ...) \ do { \ - printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_SHMEM_ACCESS_ENABLED) { \ + printf("[%s][SH][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_SHPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_CB #define VMW_CBPRN(fmt, ...) \ do { \ - printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_CB_ENABLED) { \ + printf("[%s][CB][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_CBPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_PACKETS #define VMW_PKPRN(fmt, ...) \ do { \ - printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_PACKETS_ENABLED) { \ + printf("[%s][PK][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_PKPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_WARNINGS #define VMW_WRPRN(fmt, ...) \ do { \ - printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_WARNINGS_ENABLED) { \ + printf("[%s][WR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_WRPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_ERRORS #define VMW_ERPRN(fmt, ...) \ do { \ - printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_ERRORS_ENABLED) { \ + printf("[%s][ER][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_ERPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_INTERRUPTS #define VMW_IRPRN(fmt, ...) \ do { \ - printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_INTERRUPTS_ENABLED) { \ + printf("[%s][IR][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_IRPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_CONFIG #define VMW_CFPRN(fmt, ...) \ do { \ - printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_CONFIG_ENABLED) { \ + printf("[%s][CF][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_CFPRN(fmt, ...) do {} while (0) -#endif -#ifdef VMXNET_DEBUG_RINGS #define VMW_RIPRN(fmt, ...) \ do { \ - printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ - ## __VA_ARGS__); \ + if (VMXNET_DEBUG_RINGS_ENABLED) { \ + printf("[%s][RI][%s]: " fmt "\n", VMXNET_DEVICE_NAME, __func__, \ + ## __VA_ARGS__); \ + } \ } while (0) -#else -#define VMW_RIPRN(fmt, ...) do {} while (0) -#endif #define VMXNET_MF "%02X:%02X:%02X:%02X:%02X:%02X" #define VMXNET_MA(a) (a)[0], (a)[1], (a)[2], (a)[3], (a)[4], (a)[5]
Vmxnet3 uses the following debug macro style: #ifdef SOME_DEBUG # define debug(...) do{ printf(...); } while (0) # else # define debug(...) do{ } while (0) #endif If SOME_DEBUG is undefined, then format string inside the debug macro will never be checked by compiler. Code is likely to break in the future when SOME_DEBUG is enabled because of lack of testing. This patch changes this to the following: #define debug(...) \ do { if (SOME_DEBUG_ENABLED) printf(...); } while (0) Signed-off-by: Miao Yan <yanmiaobest@gmail.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- Changes in v2: - Fix grammar error in commit log hw/net/vmxnet_debug.h | 139 +++++++++++++++++++++++++++++++------------------- 1 file changed, 86 insertions(+), 53 deletions(-)