[pisa-src] Re: r1236 - in trunk/community-operator: Makefile.am co_client.c co_common_packets.h co_server.c

On Mon, Oct 19, 2009 at 08:59:44PM +0200, Jan Marten wrote:
> 
> Log:
> Added Community-Operator protocol and renamed functions in co_server.c to 
> underscore names

You describe two completely separate things here.
Separate issues should always be committed separately.

> --- trunk/community-operator/Makefile.am      Mon Oct 19 17:37:26 2009        
> (r1235)
> +++ trunk/community-operator/Makefile.am      Mon Oct 19 20:59:44 2009        
> (r1236)
> @@ -19,6 +19,7 @@
>  INCLUDES += -I@PISA_HIPL_SRCDIR@/libhiptool
>  INCLUDES += -I@PISA_HIPL_SRCDIR@/libdht
> +INCLUDES += -I@PISA_HIPL_SRCDIR@/opendht
>  INCLUDES += -I@PISA_HIPL_SRCDIR@/hipd
>  INCLUDES += -I@PISA_HIPL_SRCDIR@/i3/i3_client
>  INCLUDES += -I@PISA_HIPL_SRCDIR@/pjproject/pjnath/include

Why?

This looks unrelated to all the other stuff that is part of this commit.

> --- trunk/community-operator/co_client.c      Mon Oct 19 17:37:26 2009        
> (r1235)
> +++ trunk/community-operator/co_client.c      Mon Oct 19 20:59:44 2009        
> (r1236)
> @@ -15,6 +17,10 @@
>  
> +static void print_deny_reason(int reason);

Unrelated to this particular commit, but who invented this ugly style of
putting forward declarations of all functions at the top of each C file?

> @@ -94,27 +137,44 @@
>   */
> -             printf("Receive failed (errno %i): %s\n", errno,
> -                    strerror(errno));
> +             PISA_ERROR("Receive failed (errno %i): %s\n", errno, 
> strerror(errno));

> --- /dev/null 00:00:00 1970   (empty, because file is newly added)
> +++ trunk/community-operator/co_common_packets.h      Mon Oct 19 20:59:44 
> 2009        (r1236)
> @@ -0,0 +1,59 @@
> +
> +typedef struct co_certificate_request
> +{
> +} __attribute__ ((packed)) co_certificate_request;
> +
> +typedef struct co_certificate_deny
> +{
> +} __attribute__ ((packed)) co_certificate_deny;
> +
> +typedef struct co_certificate
> +{
> +} __attribute__ ((packed)) co_certificate;
> +
> +typedef struct co_error
> +{
> +} __attribute__ ((packed)) co_error;
> +
> +typedef struct co_packet
> +{
> +} __attribute__ ((packed)) co_packet;

Also unrelated to this particular commit, but why are (almost) all the
structs in PISA marked as packed?

> --- trunk/community-operator/co_server.c      Mon Oct 19 17:37:26 2009        
> (r1235)
> +++ trunk/community-operator/co_server.c      Mon Oct 19 20:59:44 2009        
> (r1236)
> @@ -7,37 +7,52 @@
>  
> -             printf("socket() failed.\n");
> +             PISA_ERROR("socket() failed.\n");
>  
> -             printf("setsockopt failed with SO_REUSEADDR.\n");
> +             PISA_ERROR("setsockopt failed with SO_REUSEADDR.\n");
>  
> @@ -46,16 +61,37 @@
>  
> -             printf("bind failed.\n");
> +             PISA_ERROR("bind failed.\n");
>  
> -     printf("socket created and bound to port %i\n", port);
> +     PISA_INFO("socket created and bound to port %i\n", port);

> -             printf("sendto failed.\n");
> +             PISA_ERROR("sendto failed.\n");
>  
> -             printf("sent bytes (%i) do not match expected value (%i).\n",
> -                    sent, bytes);
> -             printf("the request has been answered.\n");
> +             PISA_ERROR("sent bytes (%i) do not match expected value 
> (%i).\n", sent,
> +                             sizeof(co_packet));
> +             PISA_INFO("the request has been answered.\n");
>  
> -             printf("You're not root. HIPD won't sign certificates.\n");
> +             PISA_ERROR("You're not root. HIPD won't sign certificates.\n");
>  
> -     printf("community operator server ready\n");
> +     PISA_INFO("community operator server ready\n");

This should have been a separate commit ..

> @@ -7,37 +7,52 @@
> -static int createServerSocket(void)
> +static int create_server_socket(void)
>  
> @@ -64,13 +100,17 @@
> -static char *getCertificateForHit(struct in6_addr *hit)
> +static char *get_certificate_for_hit(struct in6_addr *hit, int *reason)
>  
> @@ -78,110 +118,223 @@
>  
> -static void answerCertificateQuery(struct sockaddr_in6 *cl_addr)
> +static int answer_certificate_query(struct sockaddr_in6 *cl_addr)

> -     sock = createServerSocket();
> +     sock = create_server_socket();
>  
.. as should this.

> @@ -78,110 +118,223 @@
>  
> -     if (!(conn = pisa_hitlist_find(allowed, hit)))
> +     if (!(conn = pisa_hitlist_find(allowed, hit))) {
> +             *reason = DENY_HIT_NOT_ALLOWED;
>               return NULL;
> -     if (conn->expiration < not_before)
> +     }
> +     if (conn->expiration < not_before) {
> +             *reason = DENY_HIT_EXPIRED;
>               return NULL;
> -     if (conn->expiration < not_after)
> +     }
> +     if (conn->expiration < not_after) {
>               not_after = conn->expiration;
> +     }

It seems that this mostly adds useless curly braces.  The addition of
the curly braces mixed in with everything else does make the diff hard
to read though.

> +     if (users != NULL) {

if (users)

Diego

Other related posts: