| Message ID | 20170130074226.7292-1-aserdean@cloudbasesolutions.com |
|---|---|
| State | Accepted |
| Headers | show |
On 29 January 2017 at 23:42, Alin Serdean <aserdean@cloudbasesolutions.com> wrote: > From: Alin Serdean <aserdean@cloudbasesolutions.com> > > Add 'util.h' to includes otherwise the result of the function > 'ovs_format_message' will be unknown and be converted to int, > triggering an abort of vswitchd. > > Signed-off-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > Reported-at: https://github.com/openvswitch/ovs-issues/issues/123 > Reported-by: Alin Gabriel Serdean <aserdean@cloudbasesolutions.com> > --- > Intended for master and branch-2.7 > Applied, thanks. > v2: change commit message as suggested by > Gurucharan Shetty <guru@ovn.org> > --- > lib/wmi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/lib/wmi.c b/lib/wmi.c > index 632b13b..dba8022 100644 > --- a/lib/wmi.c > +++ b/lib/wmi.c > @@ -20,6 +20,7 @@ > #include <stdio.h> > #include <tchar.h> > #include "openvswitch/vlog.h" > +#include "util.h" > > VLOG_DEFINE_THIS_MODULE(wmi); > > -- > 2.10.2.windows.1 > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Jan 30, 2017 at 07:42:31AM +0000, Alin Serdean wrote: > Add 'util.h' to includes otherwise the result of the function > 'ovs_format_message' will be unknown and be converted to int, > triggering an abort of vswitchd. Is there a way to make MSVC fail the compile if a function is called without a visible prototype? That would prevent this problem. Alternatively, if it's possible to get a warning but not an error, it might be possible to add something to the appveyor build script to fail that build if any such warning is reported.
> -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Tuesday, January 31, 2017 11:35 PM > To: Alin Serdean <aserdean@cloudbasesolutions.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] windows: wmi add include > > On Mon, Jan 30, 2017 at 07:42:31AM +0000, Alin Serdean wrote: > > Add 'util.h' to includes otherwise the result of the function > > 'ovs_format_message' will be unknown and be converted to int, > > triggering an abort of vswitchd. > > Is there a way to make MSVC fail the compile if a function is called without a > visible prototype? That would prevent this problem. > > Alternatively, if it's possible to get a warning but not an error, it might be > possible to add something to the appveyor build script to fail that build if any > such warning is reported. [Alin Serdean] Thanks for the idea Ben(made me facepalm for not thinking of it). There is something like -Wimplicit-function-declaration(-Wall) and I could issue an error while compiling. I tried on Linux to add a bogus function, but make stopped during linking (from what I remember -Werror is needed to stop compiling). Since this has deep implication is it ok if I stop compiling for this type of issue? I enabled it locally and fixing current issues that we have in the code(most of them are ok since the type are int, but we need to fix them). Thanks, Alin.
On Wed, Feb 01, 2017 at 12:56:44AM +0000, Alin Serdean wrote: > > > -----Original Message----- > > From: Ben Pfaff [mailto:blp@ovn.org] > > Sent: Tuesday, January 31, 2017 11:35 PM > > To: Alin Serdean <aserdean@cloudbasesolutions.com> > > Cc: dev@openvswitch.org > > Subject: Re: [ovs-dev] [PATCH v2] windows: wmi add include > > > > On Mon, Jan 30, 2017 at 07:42:31AM +0000, Alin Serdean wrote: > > > Add 'util.h' to includes otherwise the result of the function > > > 'ovs_format_message' will be unknown and be converted to int, > > > triggering an abort of vswitchd. > > > > Is there a way to make MSVC fail the compile if a function is called without a > > visible prototype? That would prevent this problem. > > > > Alternatively, if it's possible to get a warning but not an error, it might be > > possible to add something to the appveyor build script to fail that build if any > > such warning is reported. > [Alin Serdean] Thanks for the idea Ben(made me facepalm for not thinking of it). > There is something like -Wimplicit-function-declaration(-Wall) and I could issue an error while compiling. Sounds good. > I tried on Linux to add a bogus function, but make stopped during linking (from what I remember -Werror is needed to stop compiling). By default, on Linux, we enable a lot of warnings but we do not make the compiler turn them into errors, because we assume that the common case is that some end user or distributor is building a known-good version of OVS and that they don't want the build to stop because they're using a slightly different version of the compiler that issuse warnings in slightly different circumstances. But we encourage developers to configure with --enable-Werror, which turns all warnings into errors (that break the build). That way, developers can't miss warnings that might be about important issues. I thought that we used --enable-Werror in the travis CI system too, but now I see that I'm mistaken. Maybe we should enable it there. > Since this has deep implication is it ok if I stop compiling for this type of issue? I think so, at least for this particular warning. > I enabled it locally and fixing current issues that we have in the code(most of them are ok since the type are int, but we need to fix them).
> -----Original Message----- > From: Ben Pfaff [mailto:blp@ovn.org] > Sent: Wednesday, February 1, 2017 7:36 PM > To: Alin Serdean <aserdean@cloudbasesolutions.com> > Cc: Guru Shetty <guru@ovn.org>; Sairam Venugopal > <vsairam@vmware.com>; dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v2] windows: wmi add include > > On Wed, Feb 01, 2017 at 12:56:44AM +0000, Alin Serdean wrote: > > > > > -----Original Message----- > > > From: Ben Pfaff [mailto:blp@ovn.org] > > > Sent: Tuesday, January 31, 2017 11:35 PM > > > To: Alin Serdean <aserdean@cloudbasesolutions.com> > > > Cc: dev@openvswitch.org > > > Subject: Re: [ovs-dev] [PATCH v2] windows: wmi add include > > > > > > On Mon, Jan 30, 2017 at 07:42:31AM +0000, Alin Serdean wrote: > > > > Add 'util.h' to includes otherwise the result of the function > > > > 'ovs_format_message' will be unknown and be converted to int, > > > > triggering an abort of vswitchd. > > > > > > Is there a way to make MSVC fail the compile if a function is called > > > without a visible prototype? That would prevent this problem. > > > > > > Alternatively, if it's possible to get a warning but not an error, > > > it might be possible to add something to the appveyor build script > > > to fail that build if any such warning is reported. > > [Alin Serdean] Thanks for the idea Ben(made me facepalm for not thinking > of it). > > There is something like -Wimplicit-function-declaration(-Wall) and I could > issue an error while compiling. > > Sounds good. > > > I tried on Linux to add a bogus function, but make stopped during linking > (from what I remember -Werror is needed to stop compiling). > > By default, on Linux, we enable a lot of warnings but we do not make the > compiler turn them into errors, because we assume that the common case is > that some end user or distributor is building a known-good version of OVS > and that they don't want the build to stop because they're using a slightly > different version of the compiler that issuse warnings in slightly different > circumstances. [Alin Serdean] Good point. I think on Windows the majority will use msi's for ease of use, but that is another debate > > But we encourage developers to configure with --enable-Werror, which > turns all warnings into errors (that break the build). That way, developers > can't miss warnings that might be about important issues. > > I thought that we used --enable-Werror in the travis CI system too, but now I > see that I'm mistaken. Maybe we should enable it there. [Alin Serdean] I have no idea atm, I will check it out. But even if it is enable, for sure it won't work because: https://github.com/openvswitch/ovs/blob/master/build-aux/cccl#L148 That is the wrapper that transform the gcc arguments to cl(visual studio compiler) arguments. I will spend time to try to come up with a list of matches between the two type of compiler arguments. At the moment we get level one warnings from the build, because that is the default setting. I.E.: https://ci.appveyor.com/project/blp/ovs/build/1.0.2752#L491 I will create a test appveyor account to set up the full scheme, this is too important to ignore any further. Thanks a lot for the idea and answering my questions! > > > Since this has deep implication is it ok if I stop compiling for this type of > issue? > > I think so, at least for this particular warning. > > > I enabled it locally and fixing current issues that we have in the code(most > of them are ok since the type are int, but we need to fix them).
diff --git a/lib/wmi.c b/lib/wmi.c index 632b13b..dba8022 100644 --- a/lib/wmi.c +++ b/lib/wmi.c @@ -20,6 +20,7 @@ #include <stdio.h> #include <tchar.h> #include "openvswitch/vlog.h" +#include "util.h" VLOG_DEFINE_THIS_MODULE(wmi);