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

Write-like operations: consistent check of data/timestamp #1890

Merged

Conversation

eboasson
Copy link
Contributor

Some functions, notable dds_write_ts rejected negative time stamps, a choice that I think is reasonable.

Unregister and dispose did not always check, but those occur necessarily after a write, and therefore allowing negative time stamps if they are rejected by a write operation makes no sense. So adding a check here is a bug fix.

All write-like functions taking a data argument check for a null pointer before anything else. dds_writedispose_ts failed to do this, which then results in a different error code. That bug is also fixed here.

Some functions, notable dds_write_ts rejected negative time stamps, a
choice that I think is reasonable.

Unregister and dispose did not always check, but those occur necessarily
after a write, and therefore allowing negative time stamps if they are
rejected by a write operation makes no sense.  So adding a check here is
a bug fix.

All write-like functions taking a data argument check for a null pointer
before anything else.  dds_writedispose_ts failed to do this, which then
results in a different error code.  That bug is also fixed here.

Signed-off-by: Erik Boasson <eb@ilities.com>
Copy link
Contributor

@dpotman dpotman left a comment

Choose a reason for hiding this comment

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

LGTM, just a nitpick below.

dds_return_t ret;

DDSRT_WARNING_MSVC_OFF(6387); /* Disable SAL warning on intentional misuse of the API */
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick: I think these (this one and below) disable-warnings can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. I think there are several more of those, I'll do that in a separate PR.

@eboasson eboasson merged commit 85261c1 into eclipse-cyclonedds:master Nov 30, 2023
23 checks passed
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.

2 participants