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

MDEV-35640 Protocol_text: handle errors in allocation #3699

Open
wants to merge 1 commit into
base: 10.5
Choose a base branch
from

Conversation

grooverdan
Copy link
Member

  • The Jira issue number for this PR is: MDEV-35640

Description

Protocol_text constructor allocated memory on an optional prealloc argument. This was along with its subclass Protocol_local.

Remove this functionality to an independent member function "allocate".

Replace two use with existing error handling on insufficient resources at the err label.

Protocol_local, that inherits from Protocol_text had one use that requested an allocation of 0. So it wasn't needed.

Release Notes

Improve memory allocation error handling in the protocol (text).

How can this PR be tested?

Adding error handling wasn't very invasive.

Basing the PR against the correct MariaDB version

  • This is a new feature or a refactoring, and the PR is based against the main branch.
  • This is a bug fix, and the PR is based against the earliest maintained branch in which the bug can be reproduced.

PR quality check

  • I checked the CODING_STANDARDS.md file and my PR conforms to this where appropriate.
  • For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Protocol_text constructor allocated memory on an optional prealloc argument.
This was along with its subclass Protocol_local.

Remove this functionality to an independent member function "allocate".

Replace two use with existing error handling on insufficient
resources at the err label.

Protocol_local, that inherits from Protocol_text had one use that
requested an allocation of 0. So it wasn't needed.
@grooverdan grooverdan added the MariaDB Foundation Pull requests created by MariaDB Foundation label Dec 13, 2024
@grooverdan grooverdan requested a review from knielsen December 13, 2024 01:39
@cvicentiu
Copy link
Member

Hi Daniel,

I know I wasn't asked to review this, but here's my input:

  1. True, the code did not check for errors during that "prealloc" phase, but does it actually matter?
  2. The code does check for subsequent stores, if they were succesful and if not, returns ER_OUT_OF_RESOURCES. Thus, your change, even if it makes the code seemingly safer, it doesn't offer any benefit, in practice.
  3. The class API is now more complex than before, where one should explicitly remember to prealloc to gain performance benefits.
  4. There isn't any inherent danger in not preallocating, as the store functions handle this gracefully anyway. Nor is there anything dangerous if the preallocation failed, as long as the subsequent stores have their return values checked. Hence I consider this operation happening in a "no throw" constructor to be ok.

Copy link
Member

@knielsen knielsen left a comment

Choose a reason for hiding this comment

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

Thanks, Daniel.

I think the patch looks ok, but let's not put it in old GA release 10.5. I would say earliest 11.4, or optionally even current development branch, at your choice.

Wrt. Vicentiu's comments, I don't really think the patch makes the api more complex, explicit pre-allocation seems nice and simple to me. And I do prefer that allocations are error-checked. But I do also agree that the issue is not super important. Just a nice cleanup patch to not have to worry about missing error check, hence good to push into main or 11.4 at the earliest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MariaDB Foundation Pull requests created by MariaDB Foundation
Development

Successfully merging this pull request may close these issues.

3 participants