Текстовий клієнт у форматі PDF

Учора я розмістив запит на перегляд коду . Після перегляду коду і відгуків, і після деякого сну, я вирішив, що можу зробити краще. Насправді я ввійшов у систему з наміром видалити це питання саме тоді, коли був опублікований перший огляд; так що я застряг з ним.

Це майже така ж програма, але вона є результатом набагато більше зусиль. Вона навіть включає в себе річ, яку я раніше не використовував: select . Для використання програми і попередньої програми користувач надає два аргументи: цільовий IP і цільовий порт. На відміну від останньої програми, ця програма завершується за допомогою ctrl c .

#include 
#include 
#include 
#include 
#include 
#include 
#include 

#define BUFLEN  1024
#define STDIN   0
#define STDOUT  1

void sigint_handle(int dummy);
int connect_to(char *host, int port);
int transfer(int fd_in, char *buf, int buf_len, int fd_out);

//  boolean flag for program continuation
volatile int run = 1;

//  entry point of linux ELF
int main(int args, char **argv) {
    int sd, ret_val = 0;
    fd_set fds;
    struct timeval tv;
    char buffer[BUFLEN];

    assert(args == 3);

    signal(SIGINT, sigint_handle);

    sd = connect_to(argv[1], atoi(argv[2]));

    FD_ZERO(&fds);
    tv.tv_sec = 0;
    tv.tv_usec = 10000;
    while (run) {
        FD_SET(STDIN, &fds);
        FD_SET(sd, &fds);
        ret_val = select(sd + 1, &fds, NULL, NULL, &tv);
        if (FD_ISSET(STDIN, &fds))
            ret_val = transfer(STDIN, buffer, BUFLEN, sd);
        else if (FD_ISSET(sd, &fds))
            ret_val = transfer(sd, buffer, BUFLEN, STDOUT);
        assert(ret_val == 0);
    }
    close(sd);
    return 0;
}

//  SIGINT handler
void sigint_handle(int dummy) {
    run = 0;
}

//  a very picky connect to tcp host function
int connect_to(char *host, int port) {
    struct sockaddr_in addr;
    int sd = socket(AF_INET, SOCK_STREAM, 0);
    memset(&addr, 0, sizeof(addr));
    assert(sd > -1);
    addr.sin_family = AF_INET;
    assert(inet_pton(AF_INET, host, &addr.sin_addr) > 0);
    addr.sin_port = htons(port);
    assert(connect(sd, (struct sockaddr *)&addr, sizeof(addr)) > -1);
    return sd;
}

//  transfer function - basically a one time pipe
int transfer(int fd_in, char *buf, int buf_len, int fd_out) {
    int len = read(fd_in, buf, buf_len);
    return len - write(fd_out, buf, len);
}

Я маю відзначити, що це не означає, що це зручно для користувачів. Він призначений для виконання однієї конкретної задачі як підпроцесу, і паніки в іншому випадку; знову за допомогою ctrl c для виходу.

import subprocess
import signal
import os
import socket

def dostuff():
        host = socket.gethostbyname("google.com")
        proc = subprocess.Popen(["./client " + host + " 80"],
                        stdout=subprocess.PIPE, shell=True,
                        stdin=subprocess.PIPE)
        global pid
        pid = proc.pid
        proc.stdin.write("GET /\n\n")
        while proc.poll() is None:
                print proc.stdout.readline()

if __name__ == "__main__":
        try: dostuff()
        except: os.kill(pid, signal.SIGINT)
14
@Jamal не конкретно
додано Автор kr37, джерело
Вам спеціально потрібно STDIN і STDOUT ?
додано Автор Jamal, джерело
Гаразд. Мені було цікаво, оскільки stdin і stdout вже мають однакові значення дескриптора файлу.
додано Автор Jamal, джерело

5 Відповіді

Кілька приміток:

  • As Jamal noted in the comments, your definitions of STDIN and STDOUT are somewhat useless. You can see here that using the given STDIN_FILENO and STDOUT_FILENO are already given those values. To counter your claim in the comments that they help readability, I would argue that they actually clutter up your code and should be removed.

  • Declaring run as a global variable isn't the best way to use signal handlers IMO. I would consider just testing the return value from signal for SIGINT in your while loop and quitting if that becomes true. I haven't tested this, but you shouldn't even need a sighandler_t for that.

  • The typical way that main() is written is as such:

    int main(int argc, char **argv)
    

    Note the argc, not the args. This actually stands for something ("argument count"), just like argv has a meaning as well ("argument vector").

  • All declarations should be on their own line. From Code Complete, 2d Edition, p. 759:

    With statements on their own lines, the code reads from top to bottom, instead of top to bottom and left to right. When you’re looking for a specific line of code, your eye should be able to follow the left margin of the code. It shouldn’t have to dip into each and every line just because a single line might contain two statements.

  • Always initialize your values. Right now, you've only initialized one value to 0. Initialize them all. This is to safeguard against some possible weird bugs.

  • Don't assert anything that has to do with user input. Assertions are to be used to indicate the programmer's errors, not errors of the users. Instead, test if the input is what you'd like, and if it's not then print a message telling the user what they did wrong and return an error code.

  • Use braces with your if-else conditionals please. See the Apple goto fail as for a good reason why.

10
додано
Я навіть не помітив args . Це незвично і може порушити існуючий код. Хороший улов.
додано Автор Jamal, джерело
  • Detect closed connection 1

    select promises that a read wouldn't block. This is a case when there are data, but this is also a case that the remote end is closed. In this case read immediately returns 0. Without testing for len > 0 in transfer end up in an infinite empty select/read cycle.

  • Detect closed connection 2

    The remote may close connection while you are attempting to write. This would result in SIGPIPE most likely killing the program. You need a handler, as simple as setting a flag to be tested in the main loop.

  • assert is for detecting bugs. Is not a way to handle errors.

8
додано
Щоб розгорнути трохи на assert : це форма перевірки правильності, ви повинні використовувати його для виразів, які завжди повинні бути істинними, незалежно від середовища. Див. Статтю на тему SO для отримання додаткової інформації stackoverflow.com/questions/8114008/…
додано Автор user64958, джерело

Це не може бути корисним для обліку недійсних аргументів командного рядка:

  assert (args == 3);
 

Частіше повідомляти користувача про правильні аргументи при відмові. Це повідомлення має бути надруковано до stderr , і програма повинна закінчитися деяким допустимим значенням помилки.

Ось приклад:

if (argc != 3)
{
    fprintf(stderr, "usage: %s arg1 arg2", argv[0]);
    exit(EX_USAGE);
}

(EX_USAGE is part of .)

5
додано
Я дійсно не знаходжу такі коди помилок корисними, але 0 і 1 або EXIT_ {SUCCESS, FAILURE} . По-перше, повідомлення, яке є важливим, як ви зазначили, і часто коди помилок не розглядаються, якщо ви не налаштували вашу оболонку, щоб показувати не-успішні. По-друге, sysexits.h не є портативним.
додано Автор Fred Beck, джерело

Ви припускаєте, що сокет і консоль негайно записуються, тому ваша програма може бути заблокована, якщо це ніколи не буває. Для випробування вод відповідь над тут є трохи прикладу коду.

Це також може бути використано для асинхронного підключення - потрібно чекати, поки сокет fd стане доступним для запису, тоді ви знаєте, що з'єднання встановлено. Якщо підключення виходить з ладу, fd стає читабельним, і ви отримуєте помилку, що насправді намагається прочитати.

3
додано

Кілька інших моментів:

  1. In function declarations, parameters need not have names because the compiler only needs to know what types such function may be called with.

    void sigint_handle(int dummy); 
    int connect_to(char *host, int port);
    int transfer(int fd_in, char *buf, int buf_len, int fd_out);
    
  2. In volatile int run = 1, int is not totally a good choice. The type guaranteed to be able to be accessed atomically -across asynchronous interrupts, for instance- is sig_atomic_t. See this answer, in particular:

    (§7.14 , paragraph 2) The type defined is sig_atomic_t, which is the (possibly volatile-qualified) integer type of an object that can be accessed as an atomic entity, even in the presence of asynchronous interrupts. (§7.14 , paragraph 2)

    (§7.14p5) If [a] signal occurs other than as the result of calling the abort or raise function, the behavior is undefined if the signal handler refers to any object with static storage duration other than by assigning a value to an object declared as volatile sig_atomic_t.

  3. struct sockaddr_in addr; memset(&addr, 0, sizeof(addr)) is not really needed because you want to set each field of the struct to zero, not each byte which may also include padding ones. Simply do struct sockaddr_in addr = {0}.

  4. This rearrangement looks better and more solid

    addr.sin_family = AF_INET;
    addr.sin_port   = htons(port);
    assert(inet_pton(addr.sin_family, host, &addr.sin_addr) > 0);
    
  5. Watch out for assert: as it has been said, it should not be used for error handling but for debugging bugs. assert can, indeed, do nothing (and so hide bugs) if NDEBUG is defined!

2
додано
Це правда, @jacwah. Дійсно, я сказав, що компілятори їх не потребують; люди часто роблять, альтернативно, на місці документацію про саму функцію та її параметри.
додано Автор Fred Beck, джерело
Я думаю, що все одно може бути корисно включити в декларації імена параметрів. Це не обов'язково, але це робить код більш читабельним. Імена можуть бути достатньо для того, щоб хтось читав код, щоб зрозуміти параметри. int передача (int, char *, int, int); Який із значень int знову був довжиною буфера?
додано Автор user64958, джерело
Якщо ви погоджуєтеся, я не бачу пункту цього пункту. Як я прочитав, передбачуване повідомлення полягає в тому, що операційна програма повинна опускати імена параметрів у форвардних деклараціях.
додано Автор user64958, джерело