Hello,
I accidentally discovered that Squid does not use
-Woverloaded-virtual when compiled with GCC. I found that warning useful
in other projects. Enabling it for Squid exposes one bug-in-the-making:
It looks like an 3rd Ftp::Relay::failed() argument was forgotten. The
attached patch enables the warning and probably fixes Ftp::Relay::failed().
Do we want to add this warning?
Disclaimer: The Ftp::Relay::failed() fix is correct from removing the
warning point of view, but I have not checked whether the patched code
handles non-nil ftpErr correctly. AFAICT, this method is never called
with non-nil ftpErr today, but that may change.
Thank you,
Alex.
P.S. Here is GCC's documentation for this warning, but I think its
primary value in Squid context is in detecting unintended, hidden
changes in method profiles, not avoiding compilation errors:
> -Woverloaded-virtual (C++ and Objective-C++ only)
> Warn when a function declaration hides virtual functions from a
> base class. For example, in:
>
> struct A {
> virtual void f();
> };
>
> struct B: public A {
> void f(int);
> };
>
> the "A" class version of "f" is hidden in "B", and code like:
>
> B* b;
> b->f();
>
> fails to compile.
Detect when a child method declaration hides parent virtual method.
Adding -Woverloaded-virtual exposed one problem in existing code.
=== modified file 'configure.ac'
--- configure.ac 2016-02-25 16:49:08 +0000
+++ configure.ac 2016-03-11 22:20:51 +0000
@@ -328,41 +328,41 @@ if test "x$PRESET_CFLAGS" = "x"; then
*)
;;
esac
fi
fi
dnl set squid required flags
if test "$squid_cv_compiler" = "gcc"; then
case "$squid_host_os" in
mingw)
dnl Guido Serassio ([email protected]) 20070811
dnl Using the latest MinGW (gcc 3.4.5 + mingw-runtime 3.13) cannot build with
dnl -Werror -Wmissing-prototypes -Wmissing-declarations
dnl TODO: check if the problem will be present in any other newer MinGW release.
SQUID_CFLAGS="$squid_cv_cc_option_wall -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow"
;;
*)
SQUID_CFLAGS="$squid_cv_cc_option_wall -Wpointer-arith -Wwrite-strings -Wmissing-prototypes -Wmissing-declarations -Wcomments -Wshadow"
;;
esac
- SQUID_CXXFLAGS="$squid_cv_cc_option_wall -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow"
+ SQUID_CXXFLAGS="$squid_cv_cc_option_wall -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow -Woverloaded-virtual"
else
SQUID_CFLAGS=
SQUID_CXXFLAGS=
fi
dnl CentOS (and RHEL) still define ntohs() using deprecated C++ features
SQUID_CC_REQUIRE_ARGUMENT([ac_cv_require_wno_deprecated_register],[-Werror -Wno-deprecated-register],[[#include <arpa/inet.h>]],[[int fox=ntohs(1);]])
if test "x$enable_strict_error_checking" != "xno"; then
SQUID_CFLAGS="$SQUID_CFLAGS $squid_cv_cc_option_werror"
SQUID_CXXFLAGS="$SQUID_CXXFLAGS $squid_cv_cxx_option_werror"
fi
if test "x$ac_cv_require_wno_deprecated_register" = "xyes"; then
SQUID_CXXFLAGS="$SQUID_CXXFLAGS -Wno-deprecated-register"
fi
# squid_cv_cc_arg_pipe is set by SQUID_CC_GUESS_OPTIONS
SQUID_CXXFLAGS="$SQUID_CXXFLAGS $squid_cv_cc_arg_pipe"
SQUID_CFLAGS="$SQUID_CFLAGS $squid_cv_cc_arg_pipe"
=== modified file 'src/clients/FtpRelay.cc'
--- src/clients/FtpRelay.cc 2016-02-23 08:51:22 +0000
+++ src/clients/FtpRelay.cc 2016-03-11 22:27:55 +0000
@@ -28,41 +28,41 @@ namespace Ftp
{
/// An FTP client receiving native FTP commands from our FTP server
/// (Ftp::Server), forwarding them to the next FTP hop,
/// and then relaying FTP replies back to our FTP server.
class Relay: public Ftp::Client
{
CBDATA_CLASS(Relay);
public:
explicit Relay(FwdState *const fwdState);
virtual ~Relay();
protected:
const Ftp::MasterState &master() const;
Ftp::MasterState &updateMaster();
Ftp::ServerState serverState() const { return master().serverState; }
void serverState(const Ftp::ServerState newState);
/* Ftp::Client API */
- virtual void failed(err_type error = ERR_NONE, int xerrno = 0);
+ virtual void failed(err_type error = ERR_NONE, int xerrno = 0, ErrorState *ftperr = nullptr);
virtual void dataChannelConnected(const CommConnectCbParams &io);
/* Client API */
virtual void serverComplete();
virtual void handleControlReply();
virtual void processReplyBody();
virtual void handleRequestBodyProducerAborted();
virtual bool mayReadVirginReplyBody() const;
virtual void completeForwarding();
virtual bool abortOnData(const char *reason);
/* AsyncJob API */
virtual void start();
void forwardReply();
void forwardError(err_type error = ERR_NONE, int xerrno = 0);
void failedErrorMessage(err_type error, int xerrno);
HttpReply *createHttpReply(const Http::StatusCode httpStatus, const int64_t clen = 0);
void handleDataRequest();
void startDataDownload();
@@ -239,50 +239,50 @@ Ftp::Relay::serverState(const Ftp::Serve
/**
* Ensure we do not double-complete on the forward entry.
* We complete forwarding when the response adaptation is over
* (but we may still be waiting for 226 from the FTP server) and
* also when we get that 226 from the server (and adaptation is done).
*
\todo Rewrite FwdState to ignore double completion?
*/
void
Ftp::Relay::completeForwarding()
{
debugs(9, 5, forwardingCompleted);
if (forwardingCompleted)
return;
forwardingCompleted = true;
Ftp::Client::completeForwarding();
}
void
-Ftp::Relay::failed(err_type error, int xerrno)
+Ftp::Relay::failed(err_type error, int xerrno, ErrorState *ftpErr)
{
if (!doneWithServer())
serverState(fssError);
// TODO: we need to customize ErrorState instead
if (entry->isEmpty())
failedErrorMessage(error, xerrno); // as a reply
- Ftp::Client::failed(error, xerrno);
+ Ftp::Client::failed(error, xerrno, ftpErr);
}
void
Ftp::Relay::failedErrorMessage(err_type error, int xerrno)
{
const Http::StatusCode httpStatus = failedHttpStatus(error);
HttpReply *const reply = createHttpReply(httpStatus);
entry->replaceHttpReply(reply);
EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
fwd->request->detailError(error, xerrno);
}
void
Ftp::Relay::processReplyBody()
{
debugs(9, 3, status());
if (EBIT_TEST(entry->flags, ENTRY_ABORTED)) {
/*
* probably was aborted because content length exceeds one
_______________________________________________
squid-dev mailing list
[email protected]
http://lists.squid-cache.org/listinfo/squid-dev
