Add sesman.ini FuseMountNameColonCharReplacement option

[Bad-ptr]

The colon(:) in file paths is not supported on all OSes, on others need quoting/escaping, not all software treat colons in paths adequately.
The patch modifies default behaviour by stripping colon from redirected drive mount points(which emerges from windows clients with shared drive connected), but provides config option to customize the replacement(including reverting to "good old" colons in drive names).

[matt335672]

Hi @Bad-ptr

I can see what you're doing, and I think it's a good idea.

A couple of initial thoughts, prior to a full review.

1. If FuseMountNameColonCharReplacement isn't set at all, you must preserve existing behaviour. This is so users don't have to change their configs on a minor version upgrade.
2. You don't need a char * for the config value - just a char will do. This will avoid messing about with memory allocation for this value.
3. Changing the signature of xfuse_create_share() violates the principle of least surprise, particularly if we later on modify the calling function to do something with the name we pass in. I suggest you copy this value locally if you need to.
4. _fuse_mount_name_colon_char_replace() doesn't consider array element 0 for replacement. There's nothing in the spec which suggests a client couldn't send a colon in this position.

[Bad-ptr]

If FuseMountNameColonCharReplacement isn't set at all, you must preserve existing behaviour. This is so users don't have to change their configs on a minor version upgrade.
You don't need a char * for the config value - just a char will do. This will avoid messing about with memory allocation for this value.
Changing the signature of xfuse_create_share() violates the principle of least surprise, particularly if we later on modify the calling function to do something with the name we pass in. I suggest you copy this value locally if you need to.
_fuse_mount_name_colon_char_replace() doesn't consider array element 0 for replacement. There's nothing in the spec which suggests a client couldn't send a colon in this position.

Done

Another idea is to make this option a string of pairs of chars -- the first char in pair is what to replace, the second is the replacement and the last unpaired character will be replaced by null character.

static inline char* _fuse_mount_name_colon_char_replace(const char *dirname, size_t dirnamelen)
  {
     char *newdirname = (char*)dirname;
     if (g_cfg->fuse_mount_name_colon_char_replacement && dirnamelen > 1)
      {
         char *replacepair = NULL;
          char toreplace, replaceto;
  
          newdirname = g_strdup(dirname);

          for (size_t i = dirnamelen-1; i >= 0; --i) {
             replacepair = g_cfg->fuse_mount_name_colon_char_replacement;
              while('\0' != *replacepair) {
                toreplace = replacepair[0];
                replaceto = replacepair[1];
  
                if (toreplace == newdirname[i]) {
                  newdirname[i] = replaceto;
                  break;
                }
  
                replacepair += 2;
              }
          }
      }
      return newdirname;
  }

[matt335672]

Thanks for that - looks pretty good. A few comments below. Feel free to come back to me with requests, or if you don't agree with anything I've said.

[matt335672]

You'll possibly have seen this from the CI, and I apologise for not mentioning i earlier, but this project uses opening-brace-on-new-line, e.g.:-

    if (something)
    {
    }

See https://github.com/neutrinolabs/xrdp/wiki/Coding-Style

[matt335672]

Don't need this variable.

[matt335672]

See above comment. Just use DEFAULT_FUSE_MOUNT_NAME_COLON_CHAR_REPLACEMENT here.

[matt335672]

Coding style: Function name is on new-line (e.g.):-

static inline char*
_fuse_mount_name_colon_char_replace(. . .

[matt335672]

Coding style: Replace char* with char *

[matt335672]

Awkward - this loop won't terminate as size_t is always unsigned. Also, g_strdup() may return null.

You could use g_strrchr() for this, which avoids having to pass the length in. The string is going to be pretty short, and this function is called a very few times so we're not gaining anything from passing the length in.

[matt335672]

Nice way to add the free! Suggest a comment somewhere to explain this a bit more.

[Bad-ptr]

Updated

[matt335672]

Thanks for your contribution @Bad-ptr

[matt335672]

@metalefty - worth a backport to v0.10?

[metalefty]

@matt335672 Yes, let's backport.