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

Should this be `while(is_connected(sockt))` or similar?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

What is `shell_login`? Maybe this should be an enum type with a comparison that makes sense (to me)?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Can we do the extern "C" dance in sftp.h instead? This seems a bit ugly here.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

What significance is the magic number 10 here? Use a symbolic constant?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

You can't don't need the `4` here since your specifying 4 elements in the initialization data. But 4 is special, use symbolic constant?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

We have `stfp.cpp`, but sftp.h need to be wrapped in extern "C". That seems odd.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

This looks like a copy of sftp_dirdescriptor_t from sbbs.h?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Can't use `new` here? Seems an old mix of old-style C and modern C++.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Use `snprintf()` here?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

This is converting from ASCII to UTF-8?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Would this odd code be necessary of you weren't hiding the `*` in the `sftp_file_attr_t` typedef?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

We don't store the user number of uploader (though we could, we never have, even before use of SMB), so you have to look up the usernumber from uploader's username.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Use `STR_UNKNOWN_USER` here instead?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

`Use snprintf`

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Use `snprintf`

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Doesn't appear you need to call `smb_freefilemem()` here, but it doesn't hurt anything either.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Replace repeated magic num with symbolic constant.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

I thought check for NULL before free was an anti-pattern? :-)

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

You need the variable `ret` here for some reason?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Use snprintf

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Use SAFECAT()?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

I'd prefer if these strings (/files, /home) were `#defined` somewhere and used as symbolic constants

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Magic idx values (-1, -2, -3) would be easier to understand as enums or #defines

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

The function naming convention seems inconsistent with `free_sftp_str()`

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Project headers should be `#include "filename"`, not `<filename>`

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

So why the struct tag if you're typedef'ing anyway?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

That typecast is necessary? This is C code.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Typo: large

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

I mean, it looks the same in both places, and sftp.h is a C file.

It makes sense for a C++ file to say "I'm using this C interface", but it's silly for all C headers to have to say "If you're not using a C compiler, this C file describes C code".

Also, it alerted you to the fact that the contents of sftp.cpp are not declared in this header.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

There's no significance at all, and this is the only place it's used. Ok.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

It's not the same... sftp_dirdescriptor_t has the state of a directory descriptor, and path_map_t describes a mapping between an sftp path and a "Synchronet path".

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

It's mapping Solidus to Division Slash. Both are UTF-8.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

We want it to fit in 8 chars if possible.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

If you mean the explicit nullptr check, it's not necessary no matter what I do with the typdef. Removed.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Comment updated.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

I just left enough space instead.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

So I can use offsetof.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

It's to silence warnings and/or Coverity. It is not needed.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

But... why?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Are you saying you won't accept it for merge without it, or do you want a discussion on the subject in this review? I don't want to put it in the sftp.h file, but if that's a requirement, I will.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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).

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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".

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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?

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

Then we could get rid of sbbs_t::ssh too

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net

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

All working now.

---
■ Synchronet ■ Vertrauen ■ Home of Synchronet ■ [vert/cvs/bbs].synchro.net