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 bad URL when linking VOL multiple times #98

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

mattjala
Copy link
Collaborator

@mattjala mattjala commented Nov 9, 2023

When the VOL is manually linked to multiple times, the global variable prevents connection information from being set.

It now directly checks if the connection info (base_URL) is already set.

To avoid repeating the connection info retrieval,
it now checks if the connection info (base_URL) is already set.
@mattjala mattjala added the bug Something isn't working label Nov 9, 2023
@mattjala mattjala added the testing Related to testing process label Nov 9, 2023
@@ -729,7 +728,7 @@ H5_rest_set_connection_information(void)
FILE *config_file = NULL;
herr_t ret_value = SUCCEED;

if (H5_rest_connection_info_initialized_g)
if (base_URL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since base_URL is also a global, I'm surprised it doesn't have the same issues. Are we handling it differently somewhere else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's also logic below dealing with freeing and replacing base_URL if it was already set previously. Seems like there may be some unwanted interactions with that as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think the difference is because base_URL is declared extern in rest_vol.h, while H5_rest_connection_info_initialized_g was declared statically in rest_vol.c.

The logic to free and replace base_URL doesn't make sense anymore, since setting the connection information is skipped if it's already defined. I'll remove those checks. The application won't be able to change which server it connects to during the runtime of an application, but since it can only talk to one server at a time right now anyway, I think that's probably fine. Especially since #86 will allow for different files to get different connection information in a more useful way.

@jhendersonHDF jhendersonHDF merged commit d7320bc into HDFGroup:master Nov 10, 2023
2 of 10 checks passed
@mattjala mattjala deleted the fix_manual_link_conn_info branch November 13, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing Related to testing process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants