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

Make wolfSSH_SFTPNAME_readdir overridable #552

Conversation

falemagn
Copy link
Contributor

Make wolfSSH_SFTPNAME_readdir overridable when defining WOLFSSH_USER_FILESYSTEM and SFTP_Name_readdir.

SFTP_Name_readdir has to be defined as a macro, but it can be defined to expand to its own name if also a function with the same name is defined.

What matters is that it takes 3 arguments:

1) the filesystem context as first argument;
2) WDIR* as second argument;
3) WS_SFTPNAME* as third argument

On successful execution, it returns WS_SUCCESS and the WS_SFTPNAME structure pointed by the third argument will be filled with the relevant info, otherwise a WS_* error code is returned.

@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@falemagn falemagn force-pushed the pull-reqs/9b8e9fa_Make_wolfSSH_SFTPNAME_readdir_overridable_when_defining_WOLFSSH_USER_FILESYSTEM_and_SFTP_Name_readdir branch from b628dcf to f07aa1b Compare July 21, 2023 09:53
@JacobBarthelmeh JacobBarthelmeh self-assigned this Jul 21, 2023
@JacobBarthelmeh
Copy link
Contributor

ok to test

src/wolfsftp.c Outdated
@@ -2870,6 +2870,42 @@ static int wolfSSH_SFTPNAME_readdir(WOLFSSH* ssh, WDIR* dir, WS_SFTPNAME* out,
return WS_SUCCESS;
}

#elif defined(WOLFSSH_USER_FILESYSTEM) && defined(SFTP_Name_readdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to see the overridable macro match the naming scheme of all caps with WOLFSSH_ prepended. And near the top of the if-else list not restricted to WOLFSSH_USER_FILESYSTEM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean with "near the top". Before my patch only SFTP_GetAttributes and SFTP_GetAttributes_Handle seemed to be overridable, but only if WOLFSSH_USER_FILESYSTEM is defined, I tried to stick to that, plus find a way to selectively override the specific function I needed.

Can you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

Around here for the top of the if else macro chain, https://github.com/wolfSSL/wolfssh/pull/552/files#diff-03800f8cf7db0a809cc8ba47f57f1e807aa193440295d3822eadbd4255c97118L2528. If someone is building then with say Windows, they can then still override the SFTP_NAME_READDIR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JacobBarthelmeh please, check if it's ok now.

…FILESYSTEM and SFTP_Name_readdir.

SFTP_Name_readdir has to be defined as a macro, but it can be defined to expand to its own name if also a function with the same name is defined.

What matters is that it takes 3 arguments:

    1) the filesystem context as first argument;
    2) WDIR* as second argument;
    3) WS_SFTPNAME* as third argument

On successful execution, it returns WS_SUCCESS and the WS_SFTPNAME structure pointed by the third argument will be filled with the relevant info, otherwise a WS_* error code is returned.
@falemagn falemagn force-pushed the pull-reqs/9b8e9fa_Make_wolfSSH_SFTPNAME_readdir_overridable_when_defining_WOLFSSH_USER_FILESYSTEM_and_SFTP_Name_readdir branch from f07aa1b to f9a3174 Compare October 20, 2023 06:58
…ir if WOLFSSH_SFTP_NAME_READDIR is defined, independently from WOLFSSH_USER_FILESYSTEM.
@ejohnstown ejohnstown assigned ejohnstown and unassigned falemagn May 15, 2024
@ejohnstown ejohnstown mentioned this pull request May 15, 2024
@ejohnstown
Copy link
Contributor

Closing for updated PR #691.

@ejohnstown ejohnstown closed this May 15, 2024
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.

4 participants