https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4890

Should this be `while(is_connected(sockt))` or similar?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4896

This looks like a copy of sftp_dirdescriptor_t from sbbs.h?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4901

Use `STR_UNKNOWN_USER` here instead?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4909

Use snprintf
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4910

Use SAFECAT()?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4913

The function naming convention seems inconsistent with `free_sftp_str()`
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4914

Project headers should be `#include "filename"`, not `<filename>`
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4915

So why the struct tag if you're typedef'ing anyway?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4917

Typo: large
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4933

It's true if the session type of the SSH channel is "shell". I'll change the default to false and explicitly set it after the comparison with "shell" since it's confusing.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4936

Some number is needed because without it, it would be a flexible array, and the initializer wouldn't define the size.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4937

sftp.h is from the (C) sftp library and does not describe the contents of sftp.cpp (the interface is defined here). sftp.cpp is the sftp support in sbbs_t, sftp.h is the sftp library.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4939

I'm trying to stick with the Synchronet style... I don't think there's any structs constructed in Synchronet code.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4945

It is, yes... but neither function is in this merge request. It's been on my radar for a while now though and will likely be changed one way or the other when I do the full SyncTERM sftp support.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4948

I mean... the way to test is a socket is connected is to read from it, which this loop does. I'll add a bunch of stuff to it to make it more clear I guess.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4960

Actually, sure, if we're going to go modern, may as well use new, a destructor, and unique_ptr.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4961

Because it's allowed in library and/or directory names, but SFTP reserves that character as a path separator. Since lib/dir names can't have a division slash, it's a lossless, reversible translation that looks very close or identical to what the Sysop intended by putting a slash in the name.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4967

This is now set above in the block:
```
if (tnamelen == 5 && strnicmp(tname, "shell", 5) == 0) {
shell_login = true;
session_channel = cid;
}
```
Does that make more sense now?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4970

It looks like in the code now, shell_login defaults to false and is only set to true under a specific SSH-login. Is there some other place shell_ogin is being set to true now for non-SSH logins?

I was thinking there could be something like `enum ssh_session_type { sshe_none, ssh_shell, ssh_ftp } ssh_login;`

And then `if (ssh_login != ssh_ftp)` (in the subsequent now-conditional blocks that are to only execute for non-SFTP session) seems clearer to me.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4955

It's inconsistent with all other .h files in SBBS that declare a C interface. --- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4956

But... why?
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4963

I'm just trying to understand why this C header file is "special" and not following the same pattern as the other C header files in the repo (including your own).
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4964

How about a comment that explains that? Something like: "replace slash with non-path-separator-slash if present in name".
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4965

I'm using the sftp library (and newifc) to play with library interface concepts before I start hacking up ciolib (or start on a new ciolib replacement).

One of those things is planned to be "proper" C++ support instead of having C++ code call C functions... I'm generally becoming more and more of the opinion that extern "C" blocks in C++ code indicate bad choices, and am trying to work out what the alternative good choice looks like.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4968

Here is the new setting if shell_login. Maybe doing comments here is better? --- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4971

Then we could get rid of sbbs_t::ssh too
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4976

There's a new issue with either SyncTERM or Synchronet I'm still tracking down... don't merge until this is resolved.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)

https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4979

All working now.
--- SBBSecho 3.20-Linux
* Origin: Vertrauen - [vert/cvs/bbs].synchro.net (1:103/705)