1. Forum
  2. DOVE-Net
  3. Synchronet Programming

  • Add initial support for SFTP

    From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:38 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4898

    Use `snprintf()` here?

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:39 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4903

    `Use snprintf`

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4904

    Use `snprintf`

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:40 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:20:56 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4909

    Use snprintf

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:10 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4910

    Use SAFECAT()?

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:10 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:10 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:16 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:21 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:25 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:25 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Sun Feb 25 18:21:29 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4917

    Typo: large

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:49 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:49 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:50 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:50 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:51 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:51 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4943

    Comment updated.

    ---
    ï¿­ Synchronet ï¿­ Vertrauen ï¿­ Home of Synchronet ï¿­ [vert/cvs/bbs].synchro.net
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:52 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:53 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:53 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:53 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Sun Feb 25 21:00:54 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 11:19:14 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 11:21:35 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4956

    But... why?

    ---
    þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 13:59:17 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 13:59:18 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 13:59:18 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 14:03:07 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 14:05:48 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 14:11:20 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 14:21:21 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 14:27:01 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 15:00:07 2024
    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
  • From Rob Swindell@VERT to GitLab note in main/sbbs on Mon Feb 26 15:05:54 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 17:35:21 2024
    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
  • From Deucе@VERT to GitLab note in main/sbbs on Mon Feb 26 18:56:10 2024
    https://gitlab.synchro.net/main/sbbs/-/merge_requests/415#note_4979

    All working now.

    ---
    ï¿­ Synchronet ï¿­ Vertrauen ï¿­ Home of Synchronet ï¿­ [vert/cvs/bbs].synchro.net