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
rgw: use vector for librados handles #9295
Conversation
using a vector instead of an array of pointers cleans up our initialization/shutdown logic Signed-off-by: Casey Bodley <cbodley@redhat.com>
Signed-off-by: Casey Bodley <cbodley@redhat.com>
next_rados_handle is only accessed under an exclusive handle_lock Signed-off-by: Casey Bodley <cbodley@redhat.com>
0cab969
to
1adff94
Compare
Signed-off-by: Casey Bodley <cbodley@redhat.com>
delete[] rados; | ||
} | ||
} | ||
virtual ~RGWRados() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't calling librados::Rados::shutdown()
anymore. Is that OK? Does librados::Rados::~Rados()
do that for us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks we're leaking cr_registry
but this isn't new (no regression here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
librados::Rados::~Rados()
do that for us?
librados::Rados::~Rados()
{
shutdown();
}
It looks we're leaking
cr_registry
but this isn't new (no regression here).
correct, cr_registry
is only freed in RGWRados::finalize()
. before this patch, it was leaked if the call to cr_registry->hook_to_admin_command("cr dump");
failed in RGWRados::init_rados()
. but if init_rados()
succeeds, it's fair to expect finalize()
to be called for cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cbodley this sounds familiar, I might have fixed it somewhere else
this branch went through teuthology with two valgrind issues:
the invalid read was in |
using a vector instead of an array of pointers cleans up our initialization/shutdown logic