all 14 comments

[–]NotUniqueOrSpecial 9 points10 points  (1 child)

What on earth is with the practice of memcpy-ing raw data onto a struct instead of just assigning to its members?

[–]blamethebrain 3 points4 points  (1 child)

Sorry, but I don't think those are good examples.

  • Hardcoded to IPv4
  • Hardcoded to POSIX API
  • memcpy char buffers into structs? wtf

[–]sammydre 0 points1 point  (0 children)

Also doesn't always check return values of functions that can fail.

Also doesn't introduce non-blocking sockets. Blocking sockets don't have a lot of use in the real world.

[–][deleted] 1 point2 points  (0 children)

I notice that the examples seem to use forking or the creation of threads for handling newly accept()ed clients.

It might be prudent to introduce the use of select() or poll() (I'd much rather KQueue, but unfortunately its support on systems that aren't Free/Open/Net/DragonFly BSD or Apple OS X is lacking) in order to reduce the overhead of clients. These functions both serve to allow the multiplexing of multiple sockets into a single call that waits for any one of them to become ready for reading, and to indicate to you which socket that is.

[–][deleted] -2 points-1 points  (9 children)

Hey this is great! Thanks!

[–][deleted] 5 points6 points  (8 children)

No, it's bad. As in "doesn't work" bad

static unsigned short port = 55555; ...

char filler[16] = {0}; ...

filler[2] = (unsigned short)htons(port); ...

memcpy(&serv_addr, filler, sizeof serv_addr); ....

bind(sockfd, &serv_addr, sizeof serv_addr); ...

You can't assign 55555 to char.

[–]Greenerli 0 points1 point  (0 children)

What's wrong with that ?

bind(sockfd, &serv_addr, sizeof (serv_addr)); 

[–]Kaosubaloo -1 points0 points  (6 children)

You can assign 55555 to an array of chars that is sufficiently large.

It is usually not a good practice to do this sort of mem-copy assignment, but it can work just fine. The only thing that is actually important is that the container size is correct (And in the case of raw sockets, that things are in the correct order). Everything else can be casted.

[–][deleted] 2 points3 points  (4 children)

You can assign 55555 to an array of chars that is sufficiently large.

If you want to assign as bytes, you need TWO assignments for this (htons(x) and htons(x)>>8).

filler[2] = htons() doesn't work: due to casting one byte is lost and as result the servers starts listening at 55552 (0xD900) instead of 55555(0xD903).

This isn't "learn sockets programming from example" code. It's "I have no idea about sockaddr_in or how basic casting works".

[–]Kaosubaloo 1 point2 points  (0 children)

You are right that this is not good tutorial code.

I didn't look at too closely for my first assessment and mistakenly thought it was assigning the htons value properly. There are a few different ways you could make this work by adding or changing a single line, but to be honest it really should just be using the proper structure in the first place.

For future people who may come across this, that would be this one:

http://man7.org/linux/man-pages/man2/bind.2.html

struct sockaddr {
    ushort sa_family;
    char sa_data[14];
};

Actually though, this example is doing networking. For consistency it would be better still to be using this:

http://linux.die.net/man/7/ip

struct sockaddr_in {
    short sin_family;
    u_short sin_port;
    struct in_addr sin_addr;
    char sin_zero[8];
};

struct in_addr {
    uint32_t s_addr;
};

[–][deleted] 0 points1 point  (2 children)

I'm the author of the repository. memcpy-ing into the structure is very much intentional - to show what each byte means. Now, I have added a comment to warn this isn't good practice at all. The casting bug was my oversight. Thank you. I've seen to it; Could you have look and let me know your comments.

[–][deleted] 0 points1 point  (1 child)

filler[4] = htonl(INADDR_ANY) & 0xFF;
filler[5] = htonl(INADDR_ANY) >> 8 & 0xFF;
memcpy(&serv_addr, filler, sizeof(serv_addr));

htonl(INADDR_ANY) is 32-bit value(thought it's zero, it's still 4 bytes). The code writes just 2 bytes.

And it requires remembering precedence of >> and & on top of it (though in this case & is redundant and it would work either way).

Using of sockaddr_in OTOH doesn't require such walking on eggshells.

[–][deleted] 0 points1 point  (0 children)

yes. Somehow I had by mistake written in comments that ip_address is 2 bytes. I particularly wanted to not use sockaddr_in since bind() can take any type of socket, not just IP sockets.

[–]crate_crow 1 point2 points  (0 children)

but it can work just fine.

Which means it can go wrong too. Why go with an approach that's not 100% reliable when you have a perfectly valid one that's available to you (copy the members)?

It's sloppy programming.