linux-test-project / ltp

Linux Test Project (mailing list: https://lists.linux.it/listinfo/ltp)
https://linux-test-project.readthedocs.io/
GNU General Public License v2.0
2.27k stars 1k forks source link

getsockname01: refactor with new LTP API #1151

Closed ilmanzo closed 2 months ago

ilmanzo commented 2 months ago

Refactoring of this test that verifies getsockname() returning the proper errno for various failure cases

pevik commented 2 months ago

@ilmanzo You kept the logic from the old test: test get sockfd file descriptor each time, e.g. calls setup_file() or setup_bind() each time during testing. If I'm not wrong, these 2 socket file descriptors could be static, set only once in the setup() and then passed to getsockname(), because it does not modify the socket file descriptor. Would you mind to modify it?

ilmanzo commented 2 months ago

@ilmanzo You kept the logic from the old test: test get sockfd file descriptor each time, e.g. calls setup_file() or setup_bind() each time during testing. If I'm not wrong, these 2 socket file descriptors could be static, set only once in the setup() and then passed to getsockname(), because it does not modify the socket file descriptor. Would you mind to modify it?

I'm not sure to get it; you mean to have 3 different sockfd descriptors, populated during setup, and keep them in a field of the test_cases struct ?

something like

static struct test_case {
    struct sockaddr_in *sockaddr;
    socklen_t *addrlen;
    int experrno;
    int sockfd;
    char *desc;
} tcases[] = {
    { .sockaddr = &fsin1, .addrlen = &sinlen, .experrno = EBADF, "bad file descriptor"},
    { .sockaddr = &fsin1, .addrlen = &sinlen, .experrno = ENOTSOCK, "bad file descriptor"},
    { .addrlen = &sinlen, .experrno = EFAULT, "invalid socket buffer"},
    { .sockaddr = &fsin1, .experrno = EFAULT, "invalid aligned salen"},
    { .sockaddr = &fsin1, .addrlen = (socklen_t *) 1, .experrno = EFAULT, invalid unaligned salen"},
};

static void setup(void)
{
  tcases[0].sockfd=400;
  tcases[1].sockfd=SAFE_OPEN("/dev/null", O_WRONLY);
  tcases[2].sockfd=SAFE_SOCKET(PF_INET, SOCK_STREAM, 0); // etc etc
}

I would prefer to not have a switch case inside the testing function check_getsockname(unsigned int nr) as we need 3 different sockfd values... Any suggestion ? Thanks

pevik commented 2 months ago
static void setup(void)
{
  tcases[0].sockfd=400;
  tcases[1].sockfd=SAFE_OPEN("/dev/null", O_WRONLY);
  tcases[2].sockfd=SAFE_SOCKET(PF_INET, SOCK_STREAM, 0); // etc etc
}

Here you are going to add 5 sockfd, right? One for each test, which is kind of unnecessary.

Slightly different, I would have 3 sockets as static int, similarly to static struct sockaddr_in sin0, fsin1. They will be initialized in the setup and .setup member would be replaced with new .sockfd member int sockfd (or just sock, because socket is always represented by a file descriptor).

Also, I understand that tcases specifies test unique setup. Unlike the default 0 for int, which usually means a default flag value, I consider NULL for 3rd test .sockaddr as quite important, thus I would fill it, although it's redundant (Ideal would be just to have this NULL, but we'd have to have int sock_null flag like variable).

static int sock_null, sock_bind, sock_fake;
...
static void setup(void)
{
        sock_fake = 400;
        sock_null = SAFE_OPEN("/dev/null", O_WRONLY);
        sock_bind = SAFE_SOCKET(PF_INET, SOCK_STREAM, 0);
        SAFE_BIND(sockfd, (struct sockaddr *)&sin0, sizeof(sin0));
        sinlen = sizeof(sin0);
}
...
static struct test_case {
    int sock;
    struct sockaddr_in *sockaddr;
    socklen_t *addrlen;
    int exp_err;
    char *desc;
} tcases[] = {
    { .sock = sock_fake, .sockaddr = &fsin1, .exp_err = EBADF, "bad file descriptor"},
    { .sock = sock_null, .sockaddr = &fsin1, .exp_err = ENOTSOCK, "bad file descriptor"},
    { .sock = sock_bind, .sockaddr = NULL, .exp_err = EFAULT, "invalid socket buffer"},
    { .sock = sock_bind, .sockaddr = &fsin1, .exp_err = EFAULT, "invalid aligned salen"},
    { .sock = sock_bind, .sockaddr = &fsin1, .addrlen = (socklen_t *) 1, .exp_err = EFAULT, "invalid unaligned salen"},
...
static void check_getsockname(unsigned int nr)
{
    struct test_case *tc = &tcases[nr];

    if (!tc->addrlen)
                tc->addrlen = &sinlen;

    TST_EXP_FAIL(getsockname(sockfd, (struct sockaddr *) tc->sockaddr, tc->addrlen),
                 tc->exp_err, "%s", tc->desc);
ilmanzo commented 2 months ago

} tcases[] = {
  { .sock = sock_fake, .sockaddr = &fsin1, .exp_err = EBADF, "bad file descriptor"},
  { .sock = sock_null, .sockaddr = &fsin1, .exp_err = ENOTSOCK, "bad file descriptor"},
  { .sock = sock_bind, .sockaddr = NULL, .exp_err = EFAULT, "invalid socket buffer"},
  { .sock = sock_bind, .sockaddr = &fsin1, .exp_err = EFAULT, "invalid aligned salen"},
  { .sock = sock_bind, .sockaddr = &fsin1, .addrlen = (socklen_t *) 1, .exp_err = EFAULT, "invalid unaligned salen"},
...

I'm a bit confused because using a variable to initialize a static array of struct gives me the error error: initializer element is not constant ; is that a common solution, maybe it requires some specific compiler setting ?

ilmanzo commented 2 months ago

updated using a pointer to int in the struct instead of directly the variable.