-
Notifications
You must be signed in to change notification settings - Fork 285
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
Make a separate struct Tox
containing the Messenger.
#1030
Conversation
You accidentally set your shiftwidth to 2 or something when making this commit. The spacing is triggering Travis. |
Not accidental, but this PR was also WIP, i.e. not ready for review. |
struct Tox
containing the Messenger.struct Tox
containing the Messenger.
7953bfd
to
2d7f792
Compare
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.
Reviewed 4 of 5 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @hugbubby and @sudden6)
toxcore/tox.c, line 344 at r1 (raw file):
case TOX_ERR_OPTIONS_NEW_MALLOC: SET_ERROR_PARAMETER(error, TOX_ERR_NEW_MALLOC); free(tox);
since Tox
has pointer members, I'd prefer a free_tox
function instead of free
toxcore/tox.c, line 446 at r1 (raw file):
unsigned int m_error; Messenger *const m = new_messenger(&m_options, &m_error);
Can new_messenger
fail and return a nullptr or an invalid pointer? If so consequences would be bad. Check/assert it's valid?
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.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @hugbubby and @sudden6)
toxcore/tox.c, line 344 at r1 (raw file):
Previously, sudden6 wrote…
since
Tox
has pointer members, I'd prefer afree_tox
function instead offree
That would be asymmetric. We create tox using calloc, so we free it using free. tox_new
is the constructor function for Tox
, so it should deal with appropriately freeing incomplete/inconsistent instances on error.
Fun fact: the DHT
module does this the other way around, relying on kill_dht
to clean up, but kill_dht
doesn't know how to deal with incomplete instances, so does it wrong, causing a null pointer dereference on one of the error paths (feel free to go find the path I'm talking about, but don't fix it, this is something we should fix after having a test for it).
toxcore/tox.c, line 446 at r1 (raw file):
Previously, sudden6 wrote…
Can
new_messenger
fail and return a nullptr or an invalid pointer? If so consequences would be bad. Check/assert it's valid?
It can not return an invalid non-null pointer. There would also be no way to find out in standard C (there is, on Linux). It can return nullptr, which will in turn make new_groupchats
return nullptr, which will enter the error path below. Clarifying this code is out of scope for this PR.
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.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @hugbubby and @sudden6)
toxcore/tox.c, line 344 at r1 (raw file):
Previously, iphydf wrote…
That would be asymmetric. We create tox using calloc, so we free it using free.
tox_new
is the constructor function forTox
, so it should deal with appropriately freeing incomplete/inconsistent instances on error.Fun fact: the
DHT
module does this the other way around, relying onkill_dht
to clean up, butkill_dht
doesn't know how to deal with incomplete instances, so does it wrong, causing a null pointer dereference on one of the error paths (feel free to go find the path I'm talking about, but don't fix it, this is something we should fix after having a test for it).
I'm not strongly against teaching all the destructor functions how to deal with incomplete instances and then using them for all error paths, but that should be a deliberate, documented, global, and ideally tested change in toxcore.
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.
Reviewed 1 of 5 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @hugbubby)
toxcore/tox.c, line 344 at r1 (raw file):
Previously, iphydf wrote…
I'm not strongly against teaching all the destructor functions how to deal with incomplete instances and then using them for all error paths, but that should be a deliberate, documented, global, and ideally tested change in toxcore.
Ok, I see you have thought this through a bit more than me. I think using the destructor functions on error paths in the constructor is a good idea, since then we have to handle an incomplete type at exactly one place. Needs the precautions you mentioned though.
toxcore/tox.c, line 446 at r1 (raw file):
Previously, iphydf wrote…
It can not return an invalid non-null pointer. There would also be no way to find out in standard C (there is, on Linux). It can return nullptr, which will in turn make
new_groupchats
return nullptr, which will enter the error path below. Clarifying this code is out of scope for this PR.
ok, can you then maybe put a TODO or similar here to check for non-nullptr?
This allows Tox to contain additional data on top of Messenger, making Messenger not necessarily the most top-level object. E.g. groups are built on Messenger and currently awkwardly void-pointered into it to pretend there is no cyclic dependency.
This allows Tox to contain additional data on top of Messenger, making
Messenger not necessarily the most top-level object. E.g. groups are
built on Messenger and currently awkwardly void-pointered into it to
pretend there is no cyclic dependency.
This change is