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

current target will be invalid after it is closed #52

Closed
goatshriek opened this issue Mar 23, 2019 · 6 comments
Closed

current target will be invalid after it is closed #52

goatshriek opened this issue Mar 23, 2019 · 6 comments
Labels
bug something is broken or missing good first issue something that would be simple for a newcomer to stumpless to work on help wanted external contributations encouraged

Comments

@goatshriek
Copy link
Owner

The current target is set to to the last opened target, or it can be manually set by the user using the stumpless_set_current_target function. However, if this target is closed the current target pointer is not changed, and will still point to the invalid memory.

The target closure functions need to be updated to reset the current target. It would make the most sense to set the current target to the default target when this happens. The simplest way to do this would be to add a private function to src/target.c that resets the current target, and then call this from each target closure function.

Tests can simply open a target, make sure it is the current target, and then close the target and make sure that the current target is no longer a pointer to the closed target.

The new behavior of the current target should be added to the documentation - it would make the most sense to add it to the stumpless_get_current_target function documentation.

@goatshriek goatshriek added help wanted external contributations encouraged good first issue something that would be simple for a newcomer to stumpless to work on bug something is broken or missing labels Mar 23, 2019
@annesuzuki
Copy link

Hi @goatshriek , I'd like to contribute to this project. Can I have a go at solving the issue? Thanks!

@goatshriek
Copy link
Owner Author

Absolutely! If you have any questions or just need a few hints about the project or the build environment in general, please let me know as well - I'll do my best to answer any questions or address feedback as quickly as possible.

@annesuzuki
Copy link

I'm sorry that there hasn't been progress on this project; I'm quite busy and inexperienced, but I'm hoping that won't be causing too much trouble. After following the instructions in Install.md, there were 2 errors that came up after make all. The documentation lists that perl is needed for some scripts, and considering that a new test has to be written, do I need perl? Currently, I'm running this via cygwin64.

@goatshriek
Copy link
Owner Author

No worries at all, I intended this issue as good places for folks to get their feet wet, so ask as many questions as you need to. You'll probably find some places where the documentation is lacking and needs an update.

The Perl dependency is being replaced with Ruby in version 1.5.0 - they are needed for a header inclusion checking tool (tools/check_headers) that I don't think you will need here. I'm realizing that the CONTRIBUTING.md guide doesn't mention using the latest version branch to work off of, which is something I can fix. I'll start an update to that so that it's easier to see how to get started.

I haven't tried building the project on cygwin64, but I don't think that missing Perl (or Ruby) would be causing problems with that. I'll try loading it onto a test machine and stepping through a build - can you post the errors that you're encountering in the meantime? I will see if I can re-create them and resolve the issue.

This was referenced Jan 22, 2020
@goatshriek
Copy link
Owner Author

I was able to recreate what I'm assuming are the same issue you encountered and have opened #60 to support cygwin builds. I posted a workaround there that should get you up and running. The failed network tests I referenced there are caused by my ISP faking responses to domains that don't exist - not an error in the library itself.

If those updates seem a bit overwhelming, you may have an easier time trying Visual Studio (Community Edition) or a Linux VM instead - I frequently use both of those to develop and they work well.

@goatshriek
Copy link
Owner Author

Due to the imminent release of version 1.6.0, I've started a branch to resolve this issue so that the next minor release does not have any known bugs. I apologize for scooping this out from under you, @annesuzuki. If you want to see what the code ends up looking like, you can take a look at the issue-52 branch.

If you are still interested in contributing to this project, I've added a number of issues that you can look through to see if any stand out to you. If there are any that catch your eye let me know and I can add more detail to them to help you get started. None of the issues marked as enhancements have any time urgency associated with them (unlike bug issues).

Thanks again for your contributions! The Cygwin support and branch scheme simplification (now you can just work off of the default latest branch instead of needing to switch to a version branch first) were both directly based on the problems you encountered and feedback you provided. Any other thoughts/concerns you which to add would be appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is broken or missing good first issue something that would be simple for a newcomer to stumpless to work on help wanted external contributations encouraged
Projects
None yet
Development

No branches or pull requests

2 participants