sipp icon indicating copy to clipboard operation
sipp copied to clipboard

Resolve full path of pcap which starts with '~'

Open rajeshsingh381 opened this issue 2 years ago • 8 comments

expand paths like '~/path/to/file.pcap' to '/home/myuser/path/to/file.pcap'

rajeshsingh381 avatar Feb 20 '23 12:02 rajeshsingh381

Really wish this feature gets added in next release. To that end, any suggestions/comments to code implementation are welcome, I will try my best to cater them,

rajeshsingh381 avatar Mar 01 '23 04:03 rajeshsingh381

Hi @wdoekes , could you please take a look at this PR when you find some time and offer your suggestions, if any? I am new to github community, so please also do let me know if a direct ping is inappropriate, I will refrain from in future.

rajeshsingh381 avatar Mar 08 '23 07:03 rajeshsingh381

Maybe add expanduser(). That way ~someuser/blah would also work, which will be expected.

Chatgpt was helpful enough to give me this ("can you show me a C implementation of python os.path.expanduser?"):

#include <stdlib.h>
#include <unistd.h>
#include <sys/types.h>
#include <pwd.h>

char* expanduser(char* path) {
    if (path == NULL) {
        return NULL;
    }
    
    if (path[0] == '~') {
        char* homedir = NULL;
        
        if (path[1] == '\0' || path[1] == '/') {
            homedir = getenv("HOME");
        } else {
            struct passwd* pw = getpwnam(path+1);
            
            if (pw != NULL) {
                homedir = pw->pw_dir;
            }
        }
        
        if (homedir != NULL) {
            char* expanded = malloc(strlen(homedir) + strlen(path));
            sprintf(expanded, "%s%s", homedir, path+1);
            return expanded;
        }
    }
    
    return strdup(path);
}

with these caveats:

Note that this implementation uses malloc() and free() to allocate and deallocate memory for the expanded path, so you'll need to remember to free the returned string when you're done with it.

(I'd probably prefer a version with a provided buffer.)

(Also, it uses a non-reentrant getpwnam. Look into getpwnam_r instead..)

wdoekes avatar Mar 13 '23 08:03 wdoekes

            sprintf(expanded, "%s%s", homedir, path+1);

Hehe. This can't possibly work for ~someuser unless the username length is added too; otherwise we get /home/someusersomeuser/bla

(Also.. no malloc() NULL check. But I don't want any malloc here anyway.)

wdoekes avatar Mar 13 '23 08:03 wdoekes

Thankyou @wdoekes for your suggestions, I have added support for ~/someuser/blah, ~someuser/blah and ~someuser cases and tested them. Please let me know if you have any suggestions/inputs.

rajeshsingh381 avatar Mar 14 '23 09:03 rajeshsingh381

FYI: I moved find_file() to fileutil.c where is used by both pcap and rtpstream file lookups.

wdoekes avatar Apr 08 '23 15:04 wdoekes

Sorry for inactivity, I have applied the changes you suggested, and will verify the them over the different cases you mentioned tomorrow

rajeshsingh381 avatar Apr 09 '23 10:04 rajeshsingh381

Hi, have verified the new changes, for the mentioned 3 cases. I tried to reproduce and debug the regression test failures of github-#0259 and github-#0196, but weirdly they don't happen when I run the SIPp commands manually, call is successful with 0 return code.

rajeshsingh381 avatar Apr 10 '23 12:04 rajeshsingh381

Hi, apologies for inactivity. I have fixed the regression test failures.

rajeshsingh381 avatar Jul 16 '24 10:07 rajeshsingh381

LGTM and it passes the tests.

lemenkov avatar Jul 16 '24 23:07 lemenkov

Thanks for the style fixes :+1:

I'm unfortunately unable to review this timely. Unassigning. Leaving the review+merge to someone else. E.g. @lemenkov

wdoekes avatar Jul 18 '24 09:07 wdoekes

Hi @orgads could you please review the updated diff when you get some time. Thankyou.

rajeshsingh381 avatar Jul 29 '24 18:07 rajeshsingh381

You have conflicts. Please rebase.

orgads avatar Jul 30 '24 07:07 orgads