Conversation
There was a problem hiding this comment.
Pull request overview
Adds build-time configuration options to compile out WolfSSH server or client code, with related preprocessor guards to keep SFTP/SCP buildable in reduced configurations.
Changes:
- Introduces
--disable-server/--disable-clientconfigure flags that defineNO_WOLFSSH_SERVER/NO_WOLFSSH_CLIENT. - Adds a compile-time error when both server and client are disabled.
- Updates SFTP/SCP-related preprocessor guards (including
wolfSSH_SFTP_free()directory cleanup behavior) to support client-only builds.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wolfssh/port.h | Broadens filesystem-type guard so SFTP/SCP types are available when building without server code. |
| wolfssh/internal.h | Adds a hard error if both server and client are disabled simultaneously. |
| src/wolfsftp.c | Refactors server/directory guards around SFTP code and adjusts cleanup behavior for server-disabled builds. |
| configure.ac | Adds --disable-server / --disable-client flags and maps them to -DNO_WOLFSSH_SERVER / -DNO_WOLFSSH_CLIENT. |
Comments suppressed due to low confidence (2)
src/wolfsftp.c:3438
- The directory packet handlers are now only guarded by
#ifndef NO_WOLFSSH_SERVER, but the correspondingNO_WOLFSSH_DIRguard closure was removed (#endif /* NO_WOLFSSH_DIR */). This makes the directory-specific code compile even whenNO_WOLFSSH_DIRis defined, which can breakNO_WOLFSSH_DIRbuilds (missing types/fields or unwanted directory functionality). Wrap the directory-specific handler section (e.g., the 'read a directory' / close-dir handlers) in#if !defined(NO_WOLFSSH_DIR)(or a combined!defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_DIR)), and ensure the#if/#endifpairing matches the new structure so non-directory server handlers remain available whenNO_WOLFSSH_DIRis set.
#ifndef NO_WOLFSSH_SERVER
/* Handles packet to read a directory
*
* returns WS_SUCCESS on success
src/wolfsftp.c:3565
- The directory packet handlers are now only guarded by
#ifndef NO_WOLFSSH_SERVER, but the correspondingNO_WOLFSSH_DIRguard closure was removed (#endif /* NO_WOLFSSH_DIR */). This makes the directory-specific code compile even whenNO_WOLFSSH_DIRis defined, which can breakNO_WOLFSSH_DIRbuilds (missing types/fields or unwanted directory functionality). Wrap the directory-specific handler section (e.g., the 'read a directory' / close-dir handlers) in#if !defined(NO_WOLFSSH_DIR)(or a combined!defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_DIR)), and ensure the#if/#endifpairing matches the new structure so non-directory server handlers remain available whenNO_WOLFSSH_DIRis set.
int wolfSSH_SFTP_RecvCloseDir(WOLFSSH* ssh, byte* handle, word32 handleSz)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| return WS_SUCCESS; | ||
| } | ||
| #endif /* NO_WOLFSSH_DIR */ | ||
|
|
||
| /* Handles packet to write a file |
There was a problem hiding this comment.
The directory packet handlers are now only guarded by #ifndef NO_WOLFSSH_SERVER, but the corresponding NO_WOLFSSH_DIR guard closure was removed (#endif /* NO_WOLFSSH_DIR */). This makes the directory-specific code compile even when NO_WOLFSSH_DIR is defined, which can break NO_WOLFSSH_DIR builds (missing types/fields or unwanted directory functionality). Wrap the directory-specific handler section (e.g., the 'read a directory' / close-dir handlers) in #if !defined(NO_WOLFSSH_DIR) (or a combined !defined(NO_WOLFSSH_SERVER) && !defined(NO_WOLFSSH_DIR)), and ensure the #if/#endif pairing matches the new structure so non-directory server handlers remain available when NO_WOLFSSH_DIR is set.
1. Adds --disable-server and --disable-client configure flags. Allows for compile-time exclusion of server or client code. 2. Add check to internal.h for both NO_WOLFSSH_SERVER and NO_WOLFSSH_CLIENT being set and errors. 3. In ports.h, add check for not-NO_WOLFSSH_CLIENT so SFTP/SCP filesystrem types are also available in client-only builds. 4. Update the NO_WOLFSSH_SERVER and NO_WOLFSSH_DIR guards around wolfsftp.c. Update wolfSSH_SFTP_free() to skip directory cleanup when server code is disabled. ZD #21261
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| static int SFTP_GetAttributes(void* fs, const char* fileName, | ||
| WS_SFTP_FILEATRB* atr, byte noFollow, void* heap); | ||
| #endif | ||
|
|
||
| #ifndef NO_WOLFSSH_SERVER | ||
| #if !defined(WOLFSSH_USER_FILESYSTEM) | ||
| static int SFTP_GetAttributes_Handle(WOLFSSH* ssh, byte* handle, int handleSz, |
There was a problem hiding this comment.
These forward declarations use static, but later in the file there are implementations declared without static (e.g., int SFTP_GetAttributes(...) / int SFTP_GetAttributes_Handle(...) in several platform branches). In C this is a linkage mismatch and can fail compilation (non-static declaration follows static declaration). Make the storage class consistent: either remove static from the forward declarations (if these are meant to have external linkage in at least some builds), or change all corresponding definitions to static (if these are intended to be file-private) and expose a separate non-static wrapper only when needed.
| static int SFTP_GetAttributes(void* fs, const char* fileName, | |
| WS_SFTP_FILEATRB* atr, byte noFollow, void* heap); | |
| #endif | |
| #ifndef NO_WOLFSSH_SERVER | |
| #if !defined(WOLFSSH_USER_FILESYSTEM) | |
| static int SFTP_GetAttributes_Handle(WOLFSSH* ssh, byte* handle, int handleSz, | |
| int SFTP_GetAttributes(void* fs, const char* fileName, | |
| WS_SFTP_FILEATRB* atr, byte noFollow, void* heap); | |
| #endif | |
| #ifndef NO_WOLFSSH_SERVER | |
| #if !defined(WOLFSSH_USER_FILESYSTEM) | |
| int SFTP_GetAttributes_Handle(WOLFSSH* ssh, byte* handle, int handleSz, |
| */ | ||
|
|
||
| #if defined(NO_WOLFSSH_SERVER) && defined(NO_WOLFSSH_CLIENT) | ||
| #error "You cannot disable both server and client." |
There was a problem hiding this comment.
The new #error is correct but could be more actionable for build troubleshooting. Consider including the exact macro names and the remedy (e.g., mention NO_WOLFSSH_SERVER and NO_WOLFSSH_CLIENT are both defined and that at least one must be enabled / omit one of the --disable-* flags).
| #error "You cannot disable both server and client." | |
| #error "Both NO_WOLFSSH_SERVER and NO_WOLFSSH_CLIENT are defined. Enable at least one (omit one of --disable-server or --disable-client)." |
| #if (defined(WOLFSSH_SFTP) || \ | ||
| defined(WOLFSSH_SCP) || defined(WOLFSSH_SSHD)) && \ | ||
| !defined(NO_WOLFSSH_SERVER) && \ | ||
| (!defined(NO_WOLFSSH_SERVER) || !defined(NO_WOLFSSH_CLIENT)) && \ |
There was a problem hiding this comment.
This condition is logically equivalent to !(defined(NO_WOLFSSH_SERVER) && defined(NO_WOLFSSH_CLIENT)), which is typically easier to read and matches the intent ('not both disabled'). Consider rewriting it in that form to reduce cognitive load and avoid the double-negative with !defined(...) || !defined(...).
| (!defined(NO_WOLFSSH_SERVER) || !defined(NO_WOLFSSH_CLIENT)) && \ | |
| !(defined(NO_WOLFSSH_SERVER) && defined(NO_WOLFSSH_CLIENT)) && \ |
ZD #21261