Skip to content

Commit

Permalink
Fixed bug CORE-5993 : When creation of audit log file fails, there is…
Browse files Browse the repository at this point in the history
… no error message in firebird.log
  • Loading branch information
hvlad committed Jan 25, 2019
1 parent 9c03261 commit 82e9655
Show file tree
Hide file tree
Showing 6 changed files with 77 additions and 2 deletions.
2 changes: 2 additions & 0 deletions src/include/firebird/FirebirdInterface.idl
Expand Up @@ -1165,6 +1165,8 @@ interface TraceSweepInfo : Versioned
interface TraceLogWriter : ReferenceCounted
{
uint write(const void* buf, uint size);
version: // 3.0.4 -> 3.0.5
uint write_s(Status status, const void* buf, uint size);

This comment has been minimized.

Copy link
@asfernandes

asfernandes Jan 25, 2019

Member

Please, do not invent new name convention (underline) in API methods.

This comment has been minimized.

Copy link
@hvlad

hvlad Jan 26, 2019

Author Member

It was introduced by analogue with safe CRT functions.
And it is not new, look few lines below at TracePlugin interface, for example.
Anyway, if you have good candidate i will rename, no objection :)

BTW, what do you think - is it legitimate to rename old deprecated method and use same name for the new one ?
It will not break binary compatibility and in our case it could look like:

interface TraceLogWriter : ReferenceCounted
{
	uint writeNoStatus(const void* buf, uint size);
version: // 3.0.4 -> 3.0.5
	uint write(Status status, const void* buf, uint size);
}

Also, should such deprecated method be marked as [notImplemented(true)] ?

This comment has been minimized.

Copy link
@asfernandes

asfernandes Jan 27, 2019

Member

CRT functions already uses underline convention.

I think rename the function would be ok, but there must be a way (a global define with Firebird version) so one could ifdef its code, as otherwise one including Firebird headers present in the system directory would not be able to choose and use the correct version of the function.

This comment has been minimized.

Copy link
@hvlad

hvlad Jan 28, 2019

Author Member

I decided to leave things as is

  • underlines is already used everywhere in Trace API, it is not new
  • function rename is still questionable
}

interface TraceInitInfo : Versioned
Expand Down
34 changes: 33 additions & 1 deletion src/include/firebird/IdlFbInterfaces.h
Expand Up @@ -4726,6 +4726,7 @@ namespace Firebird
struct VTable : public IReferenceCounted::VTable
{
unsigned (CLOOP_CARG *write)(ITraceLogWriter* self, const void* buf, unsigned size) throw();
unsigned (CLOOP_CARG *write_s)(ITraceLogWriter* self, IStatus* status, const void* buf, unsigned size) throw();
};

protected:
Expand All @@ -4739,13 +4740,27 @@ namespace Firebird
}

public:
static const unsigned VERSION = 3;
static const unsigned VERSION = 4;

unsigned write(const void* buf, unsigned size)
{
unsigned ret = static_cast<VTable*>(this->cloopVTable)->write(this, buf, size);
return ret;
}

template <typename StatusType> unsigned write_s(StatusType* status, const void* buf, unsigned size)
{
if (cloopVTable->version < 4)
{
StatusType::setVersionError(status, "ITraceLogWriter", cloopVTable->version, 4);
StatusType::checkException(status);
return 0;
}
StatusType::clearException(status);
unsigned ret = static_cast<VTable*>(this->cloopVTable)->write_s(this, status, buf, size);
StatusType::checkException(status);
return ret;
}
};

class ITraceInitInfo : public IVersioned
Expand Down Expand Up @@ -15206,6 +15221,7 @@ namespace Firebird
this->addRef = &Name::cloopaddRefDispatcher;
this->release = &Name::cloopreleaseDispatcher;
this->write = &Name::cloopwriteDispatcher;
this->write_s = &Name::cloopwrite_sDispatcher;
}
} vTable;

Expand All @@ -15225,6 +15241,21 @@ namespace Firebird
}
}

static unsigned CLOOP_CARG cloopwrite_sDispatcher(ITraceLogWriter* self, IStatus* status, const void* buf, unsigned size) throw()
{
StatusType status2(status);

try
{
return static_cast<Name*>(self)->Name::write_s(&status2, buf, size);
}
catch (...)
{
StatusType::catchException(&status2);
return static_cast<unsigned>(0);
}
}

static void CLOOP_CARG cloopaddRefDispatcher(IReferenceCounted* self) throw()
{
try
Expand Down Expand Up @@ -15265,6 +15296,7 @@ namespace Firebird
}

virtual unsigned write(const void* buf, unsigned size) = 0;
virtual unsigned write_s(StatusType* status, const void* buf, unsigned size) = 0;
};

template <typename Name, typename StatusType, typename Base>
Expand Down
15 changes: 15 additions & 0 deletions src/jrd/trace/TraceObjects.cpp
Expand Up @@ -503,6 +503,7 @@ class TraceLogWriterImpl FB_FINAL :

// TraceLogWriter implementation
FB_SIZE_T write(const void* buf, FB_SIZE_T size);
FB_SIZE_T write_s(CheckStatusWrapper* status, const void* buf, FB_SIZE_T size);

int release()
{
Expand Down Expand Up @@ -556,6 +557,20 @@ FB_SIZE_T TraceLogWriterImpl::write(const void* buf, FB_SIZE_T size)
return size;
}

FB_SIZE_T TraceLogWriterImpl::write_s(CheckStatusWrapper* status, const void* buf, FB_SIZE_T size)
{
try
{
return write(buf, size);
}
catch (Exception &ex)
{
ex.stuffException(status);
}

return 0;
}


/// TraceInitInfoImpl

Expand Down
17 changes: 17 additions & 0 deletions src/utilities/ntrace/PluginLogWriter.cpp
Expand Up @@ -60,7 +60,10 @@ PluginLogWriter::PluginLogWriter(const char* fileName, size_t maxSize) :
mutexName.append(m_fileName);

checkMutex("init", ISC_mutex_init(&m_mutex, mutexName.c_str()));
Guard guard(this);
#endif

reopen();
}

PluginLogWriter::~PluginLogWriter()
Expand Down Expand Up @@ -182,6 +185,20 @@ FB_SIZE_T PluginLogWriter::write(const void* buf, FB_SIZE_T size)
return written;
}

FB_SIZE_T PluginLogWriter::write_s(CheckStatusWrapper* status, const void* buf, FB_SIZE_T size)
{
try
{
return write(buf, size);
}
catch (Exception &ex)
{
ex.stuffException(status);
}

return 0;
}

void PluginLogWriter::checkErrno(const char* operation)
{
if (errno == 0)
Expand Down
1 change: 1 addition & 0 deletions src/utilities/ntrace/PluginLogWriter.h
Expand Up @@ -56,6 +56,7 @@ class PluginLogWriter FB_FINAL :

// TraceLogWriter implementation
virtual FB_SIZE_T write(const void* buf, FB_SIZE_T size);
virtual FB_SIZE_T write_s(Firebird::CheckStatusWrapper* status, const void* buf, unsigned size);

virtual int release()
{
Expand Down
10 changes: 9 additions & 1 deletion src/utilities/ntrace/TracePluginImpl.cpp
Expand Up @@ -236,7 +236,15 @@ void TracePluginImpl::logRecord(const char* action)
// TODO: implement adjusting of line breaks
// line.adjustLineBreaks();

logWriter->write(record.c_str(), record.length());
LocalStatus ls;
CheckStatusWrapper status(&ls);

logWriter->write_s(&status, record.c_str(), record.length());

if (ls.getState() & IStatus::STATE_ERRORS && ls.getErrors()[1] == isc_interface_version_too_old)
logWriter->write(record.c_str(), record.length());
else
check(&status);

record = "";
}
Expand Down

0 comments on commit 82e9655

Please sign in to comment.