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

fix for incorrect failure message with invalid reference file and crash fix #1724

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

vasudeva8
Copy link
Contributor

@vasudeva8 vasudeva8 commented Dec 21, 2023

This change is for #1723 where misleading failure message is displayed.
The error number is reset if it is due to reference file access before reporting the error, to avoid misleading message.
Another change is made to avoid crash/invalid access in hts_close.
A change in samtools will also be raised to do null check before invoking hts_close.

@daviesrob daviesrob self-assigned this Jan 2, 2024
hts.c Outdated
if (hts_opt_apply(fp, fmt->specific) != 0) {
if (((hts_opt*)fmt->specific)->opt == CRAM_OPT_REFERENCE) {
//reset error on reference file to avoid incorrect error report
errno = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than reset errno unconditionally, it would be better to take a copy before calling hts_opt_apply() and then use that to restore the old value if necessary. It might also be good to only do this if errrno is ENOENT, so we only suppress the "No such file or directory" errors and leave others alone.

E.g. something like this:

    if (fmt && fmt->specific) {
        int save_errno = errno;
        if (hts_opt_apply(fp, fmt->specific) != 0) {
            if (((hts_opt*)fmt->specific)->opt == CRAM_OPT_REFERENCE  && errno == ENOENT) {
                //reset error on reference file to avoid incorrect error report
                errno = save_errno;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made changes as discussed.

hts.c Outdated
if (hts_opt_apply(fp, fmt->specific) != 0) {
if (((hts_opt*)fmt->specific)->opt == CRAM_OPT_REFERENCE) {
const char *ref = ((hts_opt*)fmt->specific)->val.s;
hts_log_error("Couldn't load reference \"%s\" : %s",
Copy link
Member

Choose a reason for hiding this comment

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

Having tested this, it looks like refs_load_fai already reports this error, so we probably don't need to do it again here after all. Although making refs_load_fai print the error message might be useful:

samtools stats --reference /foo 9305_4#1c.cram
[E::fai_build3_core] Failed to open the file /foo
[E::refs_load_fai] Failed to open reference file '/foo'
[E::hts_open_format] Couldn't load reference "/foo" : No such file or directory
[E::hts_open_format] Failed to open file "9305_4#1c.cram"
samtools stats: failed to open "9305_4#1c.cram"

@daviesrob daviesrob merged commit b63c363 into samtools:develop Jan 10, 2024
9 checks passed
@vasudeva8 vasudeva8 deleted the stat1 branch June 5, 2024 15:33
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