Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Time based rolling policy fuzzer #461

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AdamKorcz
Copy link
Contributor

Adds a fuzzer that tests TimeBasedRollingPolicy->rollover().

Copy link
Contributor

@swebb2066 swebb2066 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preconditions should be specified in the interface but are not yet being part of the C++ language.

Do you agree that the caller has a responseability to honour preconditions?

tbrp->activateOptions(pool);
rfa->setRollingPolicy(tbrp);
rfa->activateOptions(pool);
rfa->setBufferSize(fdp.ConsumeIntegral<int>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will violate an undocumented but assumed precondition that a buffer of this size can be allocated by the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. What would a generous but still reasonable max size be here? Could we do 10,000?

Copy link
Contributor

@swebb2066 swebb2066 Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest leaving it as a default. A reasonable precondition would say "big enough for a few logging events and not bigger than the system can handle"

Copy link
Contributor

@rm5248 rm5248 Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, it seems that it is not actually allocating the amount of memory as defined by setBufferSize. It will expand up to the limit:

void BufferedWriter::write(const LogString& str, Pool& p)
{
if (m_priv->buf.length() + str.length() > m_priv->sz)
{
m_priv->out->write(m_priv->buf, p);
m_priv->buf.erase(m_priv->buf.begin(), m_priv->buf.end());
}
if (str.length() > m_priv->sz)
{
m_priv->out->write(str, p);
}
else
{
m_priv->buf.append(str);
}
}

So a random number here probably doesn't hurt, although I don't think it would actually cause any bugs either.

@AdamKorcz
Copy link
Contributor Author

Do you agree that the caller has a responseability to honour preconditions?

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants