[PATCH] itd: use libhail's anet

2010-09-23 Thread Jeff Garzik 

Updated for libhail's anet.  Not yet committed, waiting for more time to
pass.

 Makefile.am  |2 
 configure.ac |1 
 iscsiutil.h  |   52 +
 target.c |   53 ++
 target.h |3 
 util.c   |  223 ++-
 6 files changed, 57 insertions(+), 277 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d559165..d99ab0c 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -9,7 +9,7 @@ sbin_PROGRAMS   = itd
 itd_SOURCES= \
elist.h scsi_cmd_codes.h iscsiutil.h iscsi.h parameters.h target.h \
main.c iscsi.c target.c util.c parameters.c
-itd_LDADD  = @GLIB_LIBS@ @CRYPTO_LIBS@ @EVENT_LIBS@
+itd_LDADD  = @GLIB_LIBS@ @CRYPTO_LIBS@ @EVENT_LIBS@ @HAIL_LIBS@
 
 EXTRA_DIST = autogen.sh
 
diff --git a/configure.ac b/configure.ac
index 7cbbe3f..e2f6731 100644
--- a/configure.ac
+++ b/configure.ac
@@ -71,6 +71,7 @@ dnl autoconf output generation
 dnl --
 
 AM_PATH_GLIB_2_0(2.0.0)
+PKG_CHECK_MODULES(HAIL, libhail)
 
 AC_SUBST(CRYPTO_LIBS)
 AC_SUBST(EVENT_LIBS)
diff --git a/iscsiutil.h b/iscsiutil.h
index 5e5f5bf..2430c48 100644
--- a/iscsiutil.h
+++ b/iscsiutil.h
@@ -82,8 +82,8 @@
 #endif
 
 #include 
-#include 
 #include "elist.h"
+#include 
 
 /*
  * Debugging Levels
@@ -138,11 +138,11 @@ extern void iscsi_print_buffer(uint8_t *, const 
size_t);
  */
 
 struct target_session;
-struct tcp_write_state;
+struct atcp_wr_state;
 
 extern const char *sopstr(uint8_t op);
 extern int fsetflags(const char *prefix, int fd, int or_flags);
-extern int iscsi_writev(struct tcp_write_state *st,
+extern int iscsi_writev(struct atcp_wr_state *wst,
void *header, unsigned header_len,
const void *data, unsigned data_len);
 
@@ -241,53 +241,11 @@ typedef struct name { 
\
 extern size_t   strlcpy(char *, const char *, size_t);
 #endif
 
-enum {
-   TCP_MAX_WR_IOV  = 512,  /* arbitrary, pick better one */
-   TCP_MAX_WR_CNT  = 1,/* arbitrary, pick better one */
-};
-
-struct tcp_write_state {
-   int fd;
-   struct list_headwrite_q;
-   struct list_headwrite_compl_q;
-   size_t  write_cnt;  /* water level */
-   size_t  write_cnt_max;
-   boolwriting;
-   struct eventwrite_ev;
-
-   void*priv;  /* useable by any app */
-
-   /* stats */
-   unsigned long   opt_write;
-};
-
-struct tcp_write {
-   const void  *buf;   /* write buffer pointer */
-   int togo;   /* write buffer remainder */
-
-   int length; /* length for accounting */
-
-   /* callback */
-   bool(*cb)(struct tcp_write_state *, void *, bool);
-   void*cb_data;   /* data passed to cb */
-
-   struct list_headnode;
-};
-
-extern int tcp_writeq(struct tcp_write_state *st, const void *buf, unsigned 
int buflen,
-  bool (*cb)(struct tcp_write_state *, void *, bool),
-  void *cb_data);
-extern bool tcp_wr_cb_free(struct tcp_write_state *st, void *cb_data, bool 
done);
-extern void tcp_write_init(struct tcp_write_state *st, int fd);
-extern void tcp_write_exit(struct tcp_write_state *st);
-extern bool tcp_write_start(struct tcp_write_state *st);
-extern bool tcp_write_run_compl(struct tcp_write_state *st);
-
-extern void send_padding(struct tcp_write_state *st, unsigned int len_out);
+extern void send_padding(struct atcp_wr_state *wst, unsigned int len_out);
 
 extern void *header_get(void);
 extern void header_put(void *mem);
-extern bool hdr_cb_free(struct tcp_write_state *st, void *cb_data, bool done);
+extern bool hdr_cb_free(struct atcp_wr_state *, void *, bool);
 extern void hdrs_free_all(void);
 
 static inline int padding_bytes(unsigned int len_out)
diff --git a/target.c b/target.c
index 7ef7d72..be6a88b 100644
--- a/target.c
+++ b/target.c
@@ -613,9 +613,9 @@ static int task_command_t(struct target_session *sess, 
const uint8_t *header)
goto err_out_hdr;
}
 
-   tcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
+   atcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
   hdr_cb_free, rsp_header);
-   tcp_write_start(&sess->wst);
+   atcp_write_start(&sess->wst);
 
return 0;
 
@@ -839,18 +839,18 @@ static int text_command_t(struct target_session *sess, 
const uint8_t *header)
goto err_out_hdr;
}
 
-   tcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
+   atcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
   hdr_cb_free, rsp_header);
 
if (len_out) {
-   tcp_writeq(&sess->wst, t

[PATCH] tabled: use libhail's anet

2010-09-23 Thread Jeff Garzik 

Updated for move to libhail.  Not committed on main branch until sme
more time passes.

 server/bucket.c |8 -
 server/object.c |   56 +-
 server/server.c |  294 ++--
 server/status.c |3 
 server/tabled.h |   45 ++--
 5 files changed, 123 insertions(+), 283 deletions(-)

diff --git a/server/bucket.c b/server/bucket.c
index eb03e03..a81eca1 100644
--- a/server/bucket.c
+++ b/server/bucket.c
@@ -566,13 +566,13 @@ bool bucket_add(struct client *cli, const char *user, 
const char *bucket)
 bucket) < 0)
return cli_err(cli, InternalError);
 
-   rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
+   rc = atcp_writeq(&cli->wst, hdr, strlen(hdr), atcp_cb_free, hdr);
if (rc) {
free(hdr);
return true;
}
 
-   return cli_write_start(cli);
+   return atcp_write_start(&cli->wst);
 
 err_out:
rc = txn->abort(txn);
@@ -718,13 +718,13 @@ bool bucket_del(struct client *cli, const char *user, 
const char *bucket)
 hutil_time2str(timestr, sizeof(timestr), time(NULL))) < 0)
return cli_err(cli, InternalError);
 
-   rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
+   rc = atcp_writeq(&cli->wst, hdr, strlen(hdr), atcp_cb_free, hdr);
if (rc) {
free(hdr);
return true;
}
 
-   return cli_write_start(cli);
+   return atcp_write_start(&cli->wst);
 
 err_out:
rc = txn->abort(txn);
diff --git a/server/object.c b/server/object.c
index 3801e94..64cc8b6 100644
--- a/server/object.c
+++ b/server/object.c
@@ -227,13 +227,13 @@ bool object_del(struct client *cli, const char *user,
 hutil_time2str(timestr, sizeof(timestr), time(NULL))) < 0)
return cli_err(cli, InternalError);
 
-   rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
+   rc = atcp_writeq(&cli->wst, hdr, strlen(hdr), atcp_cb_free, hdr);
if (rc) {
free(hdr);
return true;
}
 
-   return cli_write_start(cli);
+   return atcp_write_start(&cli->wst);
 
 err_out:
rc = txn->abort(txn);
@@ -525,13 +525,13 @@ static bool object_put_end(struct client *cli)
return cli_err(cli, InternalError);
}
 
-   rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
+   rc = atcp_writeq(&cli->wst, hdr, strlen(hdr), atcp_cb_free, hdr);
if (rc) {
free(hdr);
return true;
}
 
-   return cli_write_start(cli);
+   return atcp_write_start(&cli->wst);
 
 err_out_rb:
rc = txn->abort(txn);
@@ -618,7 +618,8 @@ static int object_put_buf(struct client *cli, struct 
open_chunk *ochunk,
return 0;
 }
 
-bool cli_evt_http_data_in(struct client *cli, unsigned int events)
+bool cli_evt_http_data_in(struct client *cli, unsigned int events,
+ bool *invalidate_cli)
 {
ssize_t avail;
struct open_chunk *ochunk;
@@ -812,8 +813,8 @@ static bool object_put_body(struct client *cli, const char 
*user,
cli_out_end(cli);
return cli_err(cli, InternalError);
}
-   cli_writeq(cli, cont, strlen(cont), cli_cb_free, cont);
-   cli_write_start(cli);
+   atcp_writeq(&cli->wst, cont, strlen(cont), atcp_cb_free, cont);
+   atcp_write_start(&cli->wst);
}
 
avail = MIN(cli_req_avail(cli), content_len);
@@ -940,13 +941,13 @@ static bool object_put_acls(struct client *cli, const 
char *user,
return cli_err(cli, InternalError);
}
 
-   rc = cli_writeq(cli, hdr, strlen(hdr), cli_cb_free, hdr);
+   rc = atcp_writeq(&cli->wst, hdr, strlen(hdr), atcp_cb_free, hdr);
if (rc) {
free(hdr);
return true;
}
 
-   return cli_write_start(cli);
+   return atcp_write_start(&cli->wst);
 
 err_out_rb:
rc = txn->abort(txn);
@@ -990,10 +991,10 @@ void cli_in_end(struct client *cli)
cli->in_len = 0;
 }
 
-static bool object_get_more(struct client *cli, void *cb_data, bool done);
+static bool object_get_more(struct atcp_wr_state *wst, void *cb_data, bool 
done);
 
 /*
- * Return true iff cli_writeq was called. This is compatible with the
+ * Return true iff atcp_writeq was called. This is compatible with the
  * convention for cli continuation callbacks, so object_get_more can call us.
  */
 static bool object_get_poke(struct client *cli)
@@ -1026,7 +1027,7 @@ static bool object_get_poke(struct client *cli)
if (bytes == 0) {
if (!cli->in_len) {
cli_in_end(cli);
-   cli_write_start(cli);
+   atcp_write_start(&cli->wst);
}
free(buf);
return false;
@@

[PATCH] libhail: add async TCP network writing API, atcp_wr*

2010-09-23 Thread Jeff Garzik 

Just committed into libhail...  renamed the include to 'anet.h' for
'asynchronous networking'.

 include/Makefile.am |2 
 include/anet.h  |  111 +++
 lib/Makefile.am |1 
 lib/atcp.c  |  241 
 4 files changed, 354 insertions(+), 1 deletion(-)

commit 22de683a8f0566852818fac8b54ca26ae46490f0
Author: Jeff Garzik 
Date:   Thu Sep 23 20:17:56 2010 -0400

libhail: add async TCP network writing API, atcp_wr*

Signed-off-by: Jeff Garzik 

diff --git a/include/Makefile.am b/include/Makefile.am
index 234cf8a..967352a 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -5,5 +5,5 @@ EXTRA_DIST =\
 
 include_HEADERS =  \
cldc.h cld_common.h ncld.h chunkc.h chunk_msg.h \
-   hail_log.h hstor.h
+   hail_log.h hstor.h anet.h
 
diff --git a/include/anet.h b/include/anet.h
new file mode 100644
index 000..5c216c7
--- /dev/null
+++ b/include/anet.h
@@ -0,0 +1,111 @@
+#ifndef __ANET_H__
+#define __ANET_H__
+
+/*
+ * Copyright 2010 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+enum {
+   ATCP_MAX_WR_IOV = 32,   /* max iov per writev(2) */
+};
+
+typedef void (*atcp_ev_func)(int, short, void *);
+
+struct atcp_wr_ops {
+   int (*ev_wset)(void *, int, atcp_ev_func, void *);
+   int (*ev_add)(void *, const struct timeval *);
+   int (*ev_del)(void *);
+};
+
+struct atcp_wr_state {
+   int fd; /* our socket */
+
+   boolwriting;/* actively trying to write? */
+
+   size_t  write_cnt;  /* water level */
+   size_t  write_cnt_max;
+
+   struct list_headwrite_q;/* list of async writes */
+   struct list_headwrite_compl_q;  /* list of done writes */
+
+   void*priv;  /* untouched by atcp */
+
+   /* various statistics */
+   uint64_topt_write;  /* optimistic writes */
+
+   const struct atcp_wr_ops *ops;
+   void*ev_info;   /* passed to ops->ev_* */
+};
+
+typedef bool (*atcp_write_func)(struct atcp_wr_state *, void *, bool);
+
+struct atcp_write {
+   const void  *buf;   /* write buffer pointer */
+   int togo;   /* write buffer remainder */
+
+   int length; /* length for accounting */
+   atcp_write_func cb; /* callback */
+   void*cb_data;   /* data passed to cb */
+
+   struct atcp_wr_state*wst;   /* our parent */
+
+   struct list_headnode;   /* write_[compl_]q list node */
+};
+
+/* setup and teardown atcp write state */
+extern void atcp_wr_exit(struct atcp_wr_state *wst);
+extern void atcp_wr_init(struct atcp_wr_state *wst,
+ const struct atcp_wr_ops *ops, void *ev_info,
+ void *priv);
+
+/* generic write callback, that call free(cb_data2) */
+extern bool atcp_cb_free(struct atcp_wr_state *wst, void *cb_data, bool done);
+
+/* clear all write queues immediately, even if not complete */
+extern void atcp_write_free_all(struct atcp_wr_state *wst);
+
+/* complete all writes found on completion queue */
+extern bool atcp_write_run_compl(struct atcp_wr_state *wst);
+
+/* initialize internal fd, event setup */
+extern void atcp_wr_set_fd(struct atcp_wr_state *wst, int fd);
+
+/* add a buffer to the write queue */
+extern int atcp_writeq(struct atcp_wr_state *wst, const void *buf, unsigned 
int buflen,
+   atcp_write_func cb, void *cb_data);
+
+/* begin pushing write queue to socket */
+extern bool atcp_write_start(struct atcp_wr_state *wst);
+
+/* is anything on the write queue at the moment? */
+static inline bool atcp_wq_empty(struct atcp_wr_state *wst)
+{
+   return list_empty(&wst->write_q) ? true : false;
+}
+
+/* total number of octets queued at this moment */
+static inline size_t atcp_wqueued(struct atcp_wr_state *wst)
+{
+   return wst->write_cnt;
+}
+
+#endif /* __ANET_H__ */
diff --git a/lib/Makefile.am b/lib/Makefile.am
index f7b27f

Re: [tabled patch] abstract out TCP-write code

2010-09-23 Thread Jeff Garzik 

On 09/22/2010 10:37 PM, Pete Zaitcev wrote:

On Wed, 22 Sep 2010 21:26:13 -0400
Jeff Garzik  wrote:

It is a common idiom even in GLib that callbacks receive two anonymous
pointers; witness the data type GFunc's 'data' and 'user_data'
arguments:
http://library.gnome.org/devel/glib/stable/glib-Doubly-Linked-Lists.html#GFunc


There's a lot of retarged garbage in Glib, just look at their lists.
If someone smarter wrote Glib, we would not need struct list_head.


I use both list types, because there's a use case for both.  You don't 
always have the luxury of having a struct in which to embed data+next 
pointers.  Allocated strings are an excellent example.


GFunc has two parameters for a reason :)  See for example 
http://library.gnome.org/devel/glib/stable/glib-Doubly-Linked-Lists.html#g-list-foreach


It really is a common idiom, based on a common need, not just my style 
preference.  :)


Jeff


--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tabled patch] abstract out TCP-write code

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 11:28 AM, Jim Meyering wrote:

Every developer should have MALLOC_PERTURB_=N (N in 1..255) set in
his/her environment on glibc-based systems.  Almost all the time.


I heard about it a while ago, even submitted a bugzilla bug to have it 
documented adequately.  But apparently its absent from my .bash_profile. 
 Added.


Jeff



--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[itd patch] use atcp

2010-09-23 Thread Jeff Garzik 

This converts itd over to using [a local copy of] atcp.

 Makefile.am |1 
 iscsiutil.h |   52 +
 target.c|   27 +++
 target.h|2 
 util.c  |  223 ++--
 5 files changed, 29 insertions(+), 276 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d559165..9ef9e23 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -7,6 +7,7 @@ INCLUDES= @GLIB_CFLAGS@
 sbin_PROGRAMS  = itd
 
 itd_SOURCES= \
+   atcp.c atcp.h \
elist.h scsi_cmd_codes.h iscsiutil.h iscsi.h parameters.h target.h \
main.c iscsi.c target.c util.c parameters.c
 itd_LDADD  = @GLIB_LIBS@ @CRYPTO_LIBS@ @EVENT_LIBS@
diff --git a/iscsiutil.h b/iscsiutil.h
index 5e5f5bf..568a16a 100644
--- a/iscsiutil.h
+++ b/iscsiutil.h
@@ -82,8 +82,8 @@
 #endif
 
 #include 
-#include 
 #include "elist.h"
+#include "atcp.h"
 
 /*
  * Debugging Levels
@@ -138,11 +138,11 @@ extern void iscsi_print_buffer(uint8_t *, const 
size_t);
  */
 
 struct target_session;
-struct tcp_write_state;
+struct atcp_wr_state;
 
 extern const char *sopstr(uint8_t op);
 extern int fsetflags(const char *prefix, int fd, int or_flags);
-extern int iscsi_writev(struct tcp_write_state *st,
+extern int iscsi_writev(struct atcp_wr_state *wst,
void *header, unsigned header_len,
const void *data, unsigned data_len);
 
@@ -241,53 +241,11 @@ typedef struct name { 
\
 extern size_t   strlcpy(char *, const char *, size_t);
 #endif
 
-enum {
-   TCP_MAX_WR_IOV  = 512,  /* arbitrary, pick better one */
-   TCP_MAX_WR_CNT  = 1,/* arbitrary, pick better one */
-};
-
-struct tcp_write_state {
-   int fd;
-   struct list_headwrite_q;
-   struct list_headwrite_compl_q;
-   size_t  write_cnt;  /* water level */
-   size_t  write_cnt_max;
-   boolwriting;
-   struct eventwrite_ev;
-
-   void*priv;  /* useable by any app */
-
-   /* stats */
-   unsigned long   opt_write;
-};
-
-struct tcp_write {
-   const void  *buf;   /* write buffer pointer */
-   int togo;   /* write buffer remainder */
-
-   int length; /* length for accounting */
-
-   /* callback */
-   bool(*cb)(struct tcp_write_state *, void *, bool);
-   void*cb_data;   /* data passed to cb */
-
-   struct list_headnode;
-};
-
-extern int tcp_writeq(struct tcp_write_state *st, const void *buf, unsigned 
int buflen,
-  bool (*cb)(struct tcp_write_state *, void *, bool),
-  void *cb_data);
-extern bool tcp_wr_cb_free(struct tcp_write_state *st, void *cb_data, bool 
done);
-extern void tcp_write_init(struct tcp_write_state *st, int fd);
-extern void tcp_write_exit(struct tcp_write_state *st);
-extern bool tcp_write_start(struct tcp_write_state *st);
-extern bool tcp_write_run_compl(struct tcp_write_state *st);
-
-extern void send_padding(struct tcp_write_state *st, unsigned int len_out);
+extern void send_padding(struct atcp_wr_state *wst, unsigned int len_out);
 
 extern void *header_get(void);
 extern void header_put(void *mem);
-extern bool hdr_cb_free(struct tcp_write_state *st, void *cb_data, bool done);
+extern bool hdr_cb_free(struct atcp_wr_state *, void *, bool);
 extern void hdrs_free_all(void);
 
 static inline int padding_bytes(unsigned int len_out)
diff --git a/target.c b/target.c
index 7ef7d72..c68f857 100644
--- a/target.c
+++ b/target.c
@@ -613,9 +613,9 @@ static int task_command_t(struct target_session *sess, 
const uint8_t *header)
goto err_out_hdr;
}
 
-   tcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
+   atcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
   hdr_cb_free, rsp_header);
-   tcp_write_start(&sess->wst);
+   atcp_write_start(&sess->wst);
 
return 0;
 
@@ -839,18 +839,18 @@ static int text_command_t(struct target_session *sess, 
const uint8_t *header)
goto err_out_hdr;
}
 
-   tcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
+   atcp_writeq(&sess->wst, rsp_header, ISCSI_HEADER_LEN,
   hdr_cb_free, rsp_header);
 
if (len_out) {
-   tcp_writeq(&sess->wst, text_out, len_out,
-  tcp_wr_cb_free, text_out);
+   atcp_writeq(&sess->wst, text_out, len_out,
+  atcp_cb_free, text_out);
text_out = NULL;
 
send_padding(&sess->wst, len_out);
}
 
-   tcp_write_start(&sess->wst);
+   atcp_write_start(&sess->wst);
 
free(text_in);
f

[tabled patch v2] abstract out TCP-write code

2010-09-23 Thread Jeff Garzik 

Changes from v1:
- avoid referencing dead struct client (grep for 'invalidate_cli'),
  by changing FSM callback prototype.
- insert 'void *priv' member into struct atcp_wr_state, and replace
  cb_data1/cb_data2 callback parameters with (struct atcp_wr_state *, void *).
  struct client / struct session, or whatever, may be stored in
  atcp_wr_state::priv.
- minor API polishing and further abstraction

 server/Makefile.am |1 
 server/atcp.c  |  238 +++
 server/atcp.h  |  100 +++
 server/bucket.c|8 -
 server/object.c|   56 +--
 server/server.c|  268 +
 server/status.c|3 
 server/tabled.h|   46 ++---
 8 files changed, 436 insertions(+), 284 deletions(-)

diff --git a/server/Makefile.am b/server/Makefile.am
index 5b53a0a..5e0abd5 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -4,6 +4,7 @@ INCLUDES= -I$(top_srcdir)/include @GLIB_CFLAGS@ 
@HAIL_CFLAGS@
 sbin_PROGRAMS  = tabled tdbadm
 
 tabled_SOURCES = tabled.h  \
+ atcp.c atcp.h \
  bucket.c cldu.c config.c metarep.c object.c replica.c \
  server.c status.c storage.c storparse.c util.c
 tabled_LDADD   = ../lib/libtdb.a   \
diff --git a/server/atcp.c b/server/atcp.c
new file mode 100644
index 000..0050a68
--- /dev/null
+++ b/server/atcp.c
@@ -0,0 +1,238 @@
+
+/*
+ * Copyright 2010 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; see the file COPYING.  If not, write to
+ * the Free Software Foundation, 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ */
+
+#define _GNU_SOURCE
+#include "tabled-config.h"
+
+#include 
+#include 
+#include 
+#include 
+#include "atcp.h"
+
+bool atcp_cb_free(struct atcp_wr_state *wst, void *cb_data, bool done)
+{
+   free(cb_data);
+   return false;
+}
+
+static void atcp_write_complete(struct atcp_write *tmp)
+{
+   struct atcp_wr_state *wst = tmp->wst;
+
+   list_del(&tmp->node);
+   list_add_tail(&tmp->node, &wst->write_compl_q);
+}
+
+static bool atcp_write_free(struct atcp_write *tmp, bool done)
+{
+   struct atcp_wr_state *wst = tmp->wst;
+   bool rcb = false;
+
+   wst->write_cnt -= tmp->length;
+   list_del_init(&tmp->node);
+   if (tmp->cb)
+   rcb = tmp->cb(wst, tmp->cb_data, done);
+   free(tmp);
+
+   return rcb;
+}
+
+bool atcp_write_run_compl(struct atcp_wr_state *wst)
+{
+   struct atcp_write *wr;
+   bool do_loop;
+
+   do_loop = false;
+   while (!list_empty(&wst->write_compl_q)) {
+   wr = list_entry(wst->write_compl_q.next,
+   struct atcp_write, node);
+   do_loop |= atcp_write_free(wr, true);
+   }
+   return do_loop;
+}
+
+void atcp_write_free_all(struct atcp_wr_state *wst)
+{
+   struct atcp_write *wr, *tmp;
+
+   atcp_write_run_compl(wst);
+   list_for_each_entry_safe(wr, tmp, &wst->write_q, node) {
+   atcp_write_free(wr, false);
+   }
+}
+
+static bool atcp_writable(struct atcp_wr_state *wst)
+{
+   int n_iov;
+   struct atcp_write *tmp;
+   ssize_t rc;
+   struct iovec iov[ATCP_MAX_WR_IOV];
+
+   /* accumulate pending writes into iovec */
+   n_iov = 0;
+   list_for_each_entry(tmp, &wst->write_q, node) {
+   if (n_iov == ATCP_MAX_WR_IOV)
+   break;
+   /* bleh, struct iovec should declare iov_base const */
+   iov[n_iov].iov_base = (void *) tmp->buf;
+   iov[n_iov].iov_len = tmp->togo;
+   n_iov++;
+   }
+
+   /* execute non-blocking write */
+do_write:
+   rc = writev(wst->fd, iov, n_iov);
+   if (rc < 0) {
+   if (errno == EINTR)
+   goto do_write;
+   if (errno != EAGAIN)
+   goto err_out;
+   return true;
+   }
+
+   /* iterate through write queue, issuing completions based on
+* amount of data written
+*/
+   while (rc > 0) {
+   int sz;
+
+   /* get pointer to first record on list */
+   tmp = list_entry(wst->write_q.next, struct atcp_write, node);
+
+   /* mark data consumed by decreasing tmp->len */
+   sz = (tmp->togo < rc) ? tmp->togo : rc;
+   tmp->togo -= sz;
+   tmp->bu

Re: [PATCH tabled] server/server.c don't deref NULL on failed malloc

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 10:07 AM, Pete Zaitcev wrote:

On Thu, 23 Sep 2010 13:03:15 +0200
Jim Meyering  wrote:


Just noticed that sometimes tabled uses this idiom:
   if (!(key = malloc(klen + 1)))
and sometimes this:
   if ((key = malloc(klen + 1)) == NULL)
This time I used "... == NULL".


Er... The bang is Jeff's, which I try to follow always, but perhaps
one or two slipped due to opposing habit. IIRC pathtokey() was mine.
In fact tabled does not use assignments in conditions, dunno why
but it's a tradition.


I always considered assignments in conditionals as fragile and 
error-prone, and try to avoid them.  However, I occasionally break my 
own rule, such as with fgets(3) or getopt(3) loops.


And, getting back on topic, I do prefer implicit rather than explicit 
testing for zero and non-zero...  but that too is a "weak preference" 
where I won't complain much if somebody else does it.


Typically the general rule for any codebase is "try to look like the 
rest of the codebase."


Jeff



--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 03:19 PM, Jeff Garzik wrote:

3) I process patches similar to how Linus and others in the kernel do
it: "git am /path/to/mbox_of_patches" That tends to impose some
restrictions on the contents of each email.


FWIW, 'git pull' submissions are welcome.  Standard kernel-style pull 
submission style applies[1].


Jeff



[1] public git pull URL including branch name, diffstat, shortlog or 
full log of changeset summaries, and finally, the combined diff of all 
changes.




--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled] server/bucket.c: don't deref NULL upon failed malloc

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 10:03 AM, Pete Zaitcev wrote:

On Thu, 23 Sep 2010 12:53:06 +0200
Jim Meyering  wrote:


I factored out the append_const definition to avoid
having to wrap the long lines with the increased indentation.



p = s;

+#define append_const(buf, c) \
+  do { memcpy(buf, c, sizeof(c)-1); (buf) += sizeof(c)-1; } while (0)
+
tmpl = pfx_list;
while (tmpl) {


Dunno about Jeff, but I would definitely put an #undef to it
after the loop, just in case.


Yeah, or after the function.

--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 04:43 AM, Jim Meyering wrote:

From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001

From: Jim Meyering
Date: Thu, 23 Sep 2010 10:11:44 +0200
Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM


(see other email for general response to these changes, comments on 
GLib, OOM, etc.)


First off, I ACK (accept) all these changes.  Technically they appear 
correct, and I am interested in merging them.


But I request a few minor style and workflow adjustments, and a 
resubmission. Specific comments:


[style]

1) the functional style of sizeof keyword, with parens, is preferred:

-   snprintf(s, 64, "get user '%s'", user);
+   snprintf(s, sizeof s, "get user '%s'", user);


2) it is preferred to omit optional braces for singleton test+stmt style 
statements:


+   if (!pass) {
+   goto err_cmp;
+   }


[patch submission administrivia]

3) I process patches similar to how Linus and others in the kernel do 
it: "git am /path/to/mbox_of_patches"  That tends to impose some 
restrictions on the contents of each email.


In your case, while the patch descriptions and diffs themselves are 
correct, you seem to be sending one-mbox-per-email, while I'm expecting 
one-patch-per-email.  If you could tweak your process to make that 
change, that would reduce the manual labor on my part.



4) While total number of patches is not really a problem, I would 
request sweeping most of the one-and-two-liners in this series into a 
single patch, leaving perhaps only the bucket.c and status.c changes as 
standalone patches.


It's more an art and style preferences, than science, when deciding how 
to separate out changes into patches. Trying to take my cues from the 
kernel, it is preferred, for example, that bug fixes be separate from 
new features, or whitespace and cosmetic changes separate from 
functional changes.  But it is also encouraged to group similar changes 
together, if, for example, you're making a similar change across a large 
number of files.


Mailing list review-ability, useful 'git bisect' boundaries, and a 
coherent 'git shortlog' summary tend to be my guides when deciding patch 
boundaries.


Thanks!

Jeff



--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 04:43 AM, Jim Meyering wrote:


Looking at tabled's code, I see a few places where unchecked strdup
can lead to NULL deref, whereas the majority are checked carefully.
Patches for the first two I found are below -- haven't completed the job.

In most cases, I see that care is taken to detect failure and propagate
that to higher levels.  In others, due to the use of glib functions,
OOM leads to immediate (and possibly-unclean?) exit, because glib simply
calls exit on OOM.  Or perhaps tabled tells glib not to handle OOM that
way -- assuming that's possible.

This is server-style (and some of it library-grade) code, so I'm surprised
to see it relying on glib, which due to its handling of OOM errors
is often deemed unsuitable for applications that need to die
gracefully.


It's a holdover from kernel coding that I (and Pete?) obsessively check 
return values, even from memory allocation functions.  Occasionally I 
wonder if I'll ever receive an email, from an odd duck somewhere, 
thanking me for writing a server that works even VM overcommit support 
disabled.


On GLib:  it was just so darned convenient, and portable too.  It gave 
quick access to non-Linux OS's, and a bunch of convenience functions.


GLib's OOM death behavior itself is configurable via g_log*fatal*, but 
you're absolutely right that the code itself makes a standard assumption 
that memory allocation functions always succeed.


I wouldn't complain if the GLib dependency went away, but that's quite a 
project for someone...


(now, on to sending a separate email regarding the specific changes 
you've submitted here...)


Jeff


--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tabled patch] abstract out TCP-write code

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 09:57 AM, Pete Zaitcev wrote:

Side effect or not, if one applies your patch and executes
"export MALLOC_PERTURB_=43" command before "make check",
the result is a crash:


Yes, it is clear my patch re-introduces the problem.  I thanked you for 
pointing that out in the original reply.  :)


The point was, that does not invalidate the approach of storing 
write_compl_q in struct atcp_wr_state.


It merely highlights that a further fix (to the FSM? refcount client? 
etc.) is required.


Jeff


--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tabled patch] abstract out TCP-write code

2010-09-23 Thread Jim Meyering 
Pete Zaitcev wrote:
> On Thu, 23 Sep 2010 00:32:09 -0400
> Jeff Garzik  wrote:
>
>> >>>   So, we go a longer route and re-hook the list of completions
>> >>>   to a per-server global instead of a client. The patch is straight-
>> >>>   forward. The only thing we need to be careful is to make sure
>> >>>   that no outstanding completions are left in the queue before
>> >>>   freeing a client struct. This is ensured by force-running 
>> >>> completions.
>> >
>> >> Looking at this change again, I don't see how this avoids
>> >> use-after-free.  If completions exist after state change function leads
>> >> one to cli_evt_dispose() ->  cli_free(), then cli_write_run_compl() still
>> >> calls cli_write_free() with the stale 'cli' pointer.
>> >
>> > We run completions before freeing in all cases. My patch was correct.
>>
>> Logically, if completions are run before freeing in all cases, there is
>> no need to make write_compl_q global.  That was a red herring, which by
>> side effect avoided the bug with the stale 'cli' pointer.
>
> Side effect or not, if one applies your patch and executes
> "export MALLOC_PERTURB_=43" command before "make check",
> the result is a crash:
>
> Core was generated by `../server/tabled -C ../test/tabled-test.conf -E'.
> Program terminated with signal 11, Segmentation fault.
> #0  atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
> 49  struct atcp_wr_state *wst = tmp->wst;
> (gdb) where
> #0  atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
> #1  0x0040387e in atcp_write_run_compl (wst=0x15a9be8) at atcp.c:70
> #2  0x0040f20a in tcp_cli_event (fd=, events=2,
> userdata=0x15a9af0) at server.c:1227
> #3  0x7f9320aec774 in event_base_loop () from /usr/lib64/libevent-1.4.so.2
> #4  0x004107a5 in main (argc=,
> argv=) at server.c:2139
> (gdb)
>
> The existing code (with the commit that you criticized) produces no crash.
> Granted MALLOC_PERTURB_ is like SLAB debug option -- is not the normal
> operating environment -- but IMHO it is completely legitimate.

Every developer should have MALLOC_PERTURB_=N (N in 1..255) set in
his/her environment on glibc-based systems.  Almost all the time.

Maybe I should push harder to get it added to rawhide's /etc/profile.
I proposed that a few months ago:

use MALLOC_PERTURB_ ... or lose
http://thread.gmane.org/gmane.linux.redhat.fedora.devel/132690
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled] server/server.c don't deref NULL on failed malloc

2010-09-23 Thread Pete Zaitcev 
On Thu, 23 Sep 2010 13:03:15 +0200
Jim Meyering  wrote:

> Just noticed that sometimes tabled uses this idiom:
>   if (!(key = malloc(klen + 1)))
> and sometimes this:
>   if ((key = malloc(klen + 1)) == NULL)
> This time I used "... == NULL".

Er... The bang is Jeff's, which I try to follow always, but perhaps
one or two slipped due to opposing habit. IIRC pathtokey() was mine.
In fact tabled does not use assignments in conditions, dunno why
but it's a tradition.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled] server/bucket.c: don't deref NULL upon failed malloc

2010-09-23 Thread Pete Zaitcev 
On Thu, 23 Sep 2010 12:53:06 +0200
Jim Meyering  wrote:

> I factored out the append_const definition to avoid
> having to wrap the long lines with the increased indentation.

>   p = s;
> 
> +#define append_const(buf, c) \
> +  do { memcpy(buf, c, sizeof(c)-1); (buf) += sizeof(c)-1; } while (0)
> +
>   tmpl = pfx_list;
>   while (tmpl) {

Dunno about Jeff, but I would definitely put an #undef to it
after the loop, just in case.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tabled patch] abstract out TCP-write code

2010-09-23 Thread Pete Zaitcev 
On Thu, 23 Sep 2010 00:32:09 -0400
Jeff Garzik  wrote:

> >>>   So, we go a longer route and re-hook the list of completions
> >>>   to a per-server global instead of a client. The patch is straight-
> >>>   forward. The only thing we need to be careful is to make sure
> >>>   that no outstanding completions are left in the queue before
> >>>   freeing a client struct. This is ensured by force-running 
> >>> completions.
> >
> >> Looking at this change again, I don't see how this avoids
> >> use-after-free.  If completions exist after state change function leads
> >> one to cli_evt_dispose() ->  cli_free(), then cli_write_run_compl() still
> >> calls cli_write_free() with the stale 'cli' pointer.
> >
> > We run completions before freeing in all cases. My patch was correct.
> 
> Logically, if completions are run before freeing in all cases, there is 
> no need to make write_compl_q global.  That was a red herring, which by 
> side effect avoided the bug with the stale 'cli' pointer.

Side effect or not, if one applies your patch and executes
"export MALLOC_PERTURB_=43" command before "make check",
the result is a crash:

Core was generated by `../server/tabled -C ../test/tabled-test.conf -E'.
Program terminated with signal 11, Segmentation fault.
#0  atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
49  struct atcp_wr_state *wst = tmp->wst;
(gdb) where
#0  atcp_write_free (tmp=0x2b2b2b2b2b2b2afb, done=true) at atcp.c:49
#1  0x0040387e in atcp_write_run_compl (wst=0x15a9be8) at atcp.c:70
#2  0x0040f20a in tcp_cli_event (fd=, events=2,
userdata=0x15a9af0) at server.c:1227
#3  0x7f9320aec774 in event_base_loop () from /usr/lib64/libevent-1.4.so.2
#4  0x004107a5 in main (argc=,
argv=) at server.c:2139
(gdb) 

The existing code (with the commit that you criticized) produces no crash.
Granted MALLOC_PERTURB_ is like SLAB debug option -- is not the normal
operating environment -- but IMHO it is completely legitimate.

-- Pete
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled] server/server.c don't deref NULL on failed malloc

2010-09-23 Thread Jim Meyering 


Signed-off-by: Jim Meyering 
---
Just noticed that sometimes tabled uses this idiom:
  if (!(key = malloc(klen + 1)))
and sometimes this:
  if ((key = malloc(klen + 1)) == NULL)
This time I used "... == NULL".

 server/server.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/server/server.c b/server/server.c
index 2b4ef1c..3cec5ae 100644
--- a/server/server.c
+++ b/server/server.c
@@ -306,7 +306,8 @@ static char *pathtokey(const char *path)
return NULL;
klen = end - path;

-   key = malloc(klen + 1);
+   if ((key = malloc(klen + 1)) == NULL)
+   return NULL;
memcpy(key, path, klen);
key[klen] = 0;

--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled] server/bucket.c: don't deref NULL upon failed malloc

2010-09-23 Thread Jim Meyering 
This was a little more work to fix.
Can't return early without leaking, so I made all
of those P derefs conditional on P being non-NULL.
I factored out the append_const definition to avoid
having to wrap the long lines with the increased indentation.

>From b354befe9f7fe754c5fe012e424ccec84ef4e96d Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 12:47:32 +0200
Subject: [PATCH tabled] server/bucket.c: don't deref NULL upon failed malloc


Signed-off-by: Jim Meyering 
---
 server/bucket.c |   24 +++-
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/server/bucket.c b/server/bucket.c
index eb03e03..a4af385 100644
--- a/server/bucket.c
+++ b/server/bucket.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -788,28 +788,34 @@ static GList *bucket_list_pfx(GList *content, GHashTable 
*common_pfx,
s = malloc(cpfx_len);
p = s;

+#define append_const(buf, c) \
+  do { memcpy(buf, c, sizeof(c)-1); (buf) += sizeof(c)-1; } while (0)
+
tmpl = pfx_list;
while (tmpl) {
prefix = (char *) tmpl->data;
pfx_len = strlen(prefix);

-   memcpy(p, optag, sizeof(optag)-1);  p += sizeof(optag)-1;
-   memcpy(p, pfoptag, sizeof(pfoptag)-1);  p += sizeof(pfoptag)-1;
-   memcpy(p, prefix, pfx_len);  p += pfx_len;
-   memcpy(p, delim, delim_len);  p += delim_len;
-   memcpy(p, pfedtag, sizeof(pfedtag)-1);  p += sizeof(pfedtag)-1;
-   memcpy(p, edtag, sizeof(edtag)-1);  p += sizeof(edtag)-1;
+   if (p) {
+   append_const(p, optag);
+   append_const(p, pfoptag);
+   memcpy(p, prefix, pfx_len);  p += pfx_len;
+   memcpy(p, delim, delim_len);  p += delim_len;
+   append_const(p, pfedtag);
+   append_const(p, edtag);
+   }

free(prefix);

tmpl = tmpl->next;
}
-   *p = 0;
+   if (p)
+   *p = 0;

free(delim);
g_list_free(pfx_list);

-   return g_list_append(content, s);
+   return s ? g_list_append(content, s) : content;
 }

 struct bucket_list_info {
--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled] server/status.c: don't deref NULL on failed strdup

2010-09-23 Thread Jim Meyering 

>From aed01fa24688257fdf6f3c5ecf6983f61b8749f8 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 11:12:27 +0200
Subject: [PATCH tabled] server/status.c: don't deref NULL on failed strdup

Use a non-malloc'd string upon failed strdup so
we don't have to diagnose failure.

Signed-off-by: Jim Meyering 
---
 server/status.c |8 +---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/server/status.c b/server/status.c
index e9fbb38..7d4cc9a 100644
--- a/server/status.c
+++ b/server/status.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -158,13 +158,14 @@ bool stat_evt_http_req(struct client *cli, unsigned int 
events)
char *path = NULL;
// int rc;
bool rcb;
+   char *root = (char *) "/";

/* grab useful headers */
// content_len_str = hreq_hdr(req, "content-length");

path = strdup(req->uri.path);
if (!path)
-   path = strdup("/");
+   path = root;

if (debugging)
applog(LOG_INFO, "%s: status method %s, path '%s'",
@@ -195,6 +196,7 @@ bool stat_evt_http_req(struct client *cli, unsigned int 
events)
rcb = stat_err(cli, InvalidArgument);
}

-   free(path);
+   if (path != root);
+   free(path);
return rcb;
 }
--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled] server/server.c: don't deref NULL on failed strdup

2010-09-23 Thread Jim Meyering 

>From 6193423b13197c63ff36d01438a23b4b1278825b Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 11:02:36 +0200
Subject: [PATCH tabled] server/server.c: don't deref NULL on failed strdup

Here, pathtokey does handle a NULL argument, but the following
applog/printf would dereference NULL (of course, that's ok
with glibc's printf machinery, but not for other C libraries).

Signed-off-by: Jim Meyering 
---
 server/server.c |3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/server/server.c b/server/server.c
index 87681a9..2b4ef1c 100644
--- a/server/server.c
+++ b/server/server.c
@@ -1000,7 +1000,8 @@ static bool cli_evt_http_req(struct client *cli, unsigned 
int events)
if (debugging)
applog(LOG_INFO,
   "%s: method %s, path '%s', key '%s', bucket '%s'",
-  cli->addr_host, method, path, key, bucket);
+  cli->addr_host, method, path ? path : "",
+  key, bucket);

if (auth) {
err = authcheck(&cli->req, buck_in_path? NULL: bucket, auth,
--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled 2/2] server/server.c: don't deref NULL on failed strdup

2010-09-23 Thread Jim Meyering 
This first one is so small it barely deserved a commit.
The second fixes a bug.

>From 8e6255aa5cddd7bd58d7e4dbd2ecc81f84c51ea2 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 10:47:22 +0200
Subject: [PATCH tabled 1/2] server/server.c: use "sizeof s" rather than 
equivalent "64"


Signed-off-by: Jim Meyering 
---
 server/server.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/server/server.c b/server/server.c
index 3398026..a1af4f0 100644
--- a/server/server.c
+++ b/server/server.c
@@ -356,7 +356,7 @@ static int authcheck(struct http_req *req, char 
*extra_bucket,
if (rc != DB_NOTFOUND) {
char s[64];

-   snprintf(s, 64, "get user '%s'", user);
+   snprintf(s, sizeof s, "get user '%s'", user);
tdbrep.tdb.passwd->err(tdbrep.tdb.passwd, rc, s);
}
} else {
--
1.7.3.234.g7bba3


>From cf1f535adee9d93769cac5754c108ea9dac8a5c2 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 10:56:01 +0200
Subject: [PATCH tabled 2/2] server/server.c: don't deref NULL on failed strdup

Otherwise, hreq_sign(..., ..., NULL,...) would end up
calling strlen(NULL).

Signed-off-by: Jim Meyering 
---
 server/server.c |4 
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/server/server.c b/server/server.c
index a1af4f0..87681a9 100644
--- a/server/server.c
+++ b/server/server.c
@@ -363,6 +363,10 @@ static int authcheck(struct http_req *req, char 
*extra_bucket,
pass = val.data;
}

+   if (!pass) {
+   goto err_cmp;
+   }
+
hreq_sign(req, extra_bucket, pass, b64sig);
free(pass);

--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

2010-09-23 Thread Jim Meyering 

Looking at tabled's code, I see a few places where unchecked strdup
can lead to NULL deref, whereas the majority are checked carefully.
Patches for the first two I found are below -- haven't completed the job.

In most cases, I see that care is taken to detect failure and propagate
that to higher levels.  In others, due to the use of glib functions,
OOM leads to immediate (and possibly-unclean?) exit, because glib simply
calls exit on OOM.  Or perhaps tabled tells glib not to handle OOM that
way -- assuming that's possible.

This is server-style (and some of it library-grade) code, so I'm surprised
to see it relying on glib, which due to its handling of OOM errors
is often deemed unsuitable for applications that need to die
gracefully.

I'm not 100% sure that each of these strdup
failures will lead inevitably to a NULL deref, but a quick
manual trace suggested it's possible, so...

>From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 10:11:44 +0200
Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM


Signed-off-by: Jim Meyering 
---
 server/config.c |7 +--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/server/config.c b/server/config.c
index f94886e..a58a0e6 100644
--- a/server/config.c
+++ b/server/config.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2009 Red Hat, Inc.
+ * Copyright 2009, 2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -436,7 +436,10 @@ void read_config(void)

memset(&ctx, 0, sizeof(struct config_context));

-   tabled_srv.port = strdup("8080");
+   if (!(tabled_srv.port = strdup("8080"))) {
+   applog(LOG_ERR, "no core");
+   exit(1);
+   }

if (!g_file_get_contents(tabled_srv.config, &text, &len, NULL)) {
applog(LOG_ERR, "failed to read config file %s",
--
1.7.3.234.g7bba3


>From 725ff27469746639f0da098feab6c3d0a28e7b30 Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 10:25:15 +0200
Subject: [PATCH tabled 2/2] server/object.c: don't deref NULL on OOM


Signed-off-by: Jim Meyering 
---
 server/object.c |7 ++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/server/object.c b/server/object.c
index 3801e94..1f2f68f 100644
--- a/server/object.c
+++ b/server/object.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -801,6 +801,11 @@ static bool object_put_body(struct client *cli, const char 
*user,
cli->out_objid = objid;
cli->out_user = strdup(user);

+   if (!cli->out_bucket || !cli->out_key || !cli->out_user) {
+   applog(LOG_ERR, "OOM in object_put_body");
+   return cli_err(cli, InternalError);
+   }
+
/* handle Expect: 100-continue header, by unconditionally
 * requesting that they continue.
 */
--
1.7.3.234.g7bba3
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled] server/server.c (net_write_port): Don't ignore write error.

2010-09-23 Thread Jim Meyering 
Jeff Garzik wrote:

> On 09/23/2010 03:55 AM, Jim Meyering wrote:
>> Better safe than sorry...
>> Unreported write failures can be unpleasant.
>> I fixed the one below so that a failure indication
>> can propagate up the call tree.  You might also want to
>> report the failure to stderr.
>>
>> I let my editor automatically update the copyright date
>> and remove trailing spaces.
>> If you'd rather separate those from the fix,
>> let me know and I can adjust and resend.
>
> Patch applied, thanks.
>
> The typical preference is to receive whitespace and other cosmetic
> changes in a separate patch, thereby highlighting the functional
> changes.
>
> But we're not so strict here that I would reject an otherwise useful
> patch...
>
> Also FWIW, we're not very strict about reproducing the GCC-ish
> (GNU-ish?) style of "$FILENAME ($FUNCTION):" in each changelog --
> though you're certainly welcome to continue, if that's your
> preference.

Yes, the function name is added automatically when I hit C-4-a.
Easy to omit from now on.

> Given that "git show $COMMIT" will give you filename and
> per-diff-chunk function names, reproducing that in the git changelog
> entry seems somewhat redundant.  A simple, English-language summary of
> the change is fine.  Just a style tip, though, feel free to ignore!
> :)

Tips are always most welcome.  Thanks.
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH tabled] server/server.c (net_write_port): Don't ignore write error.

2010-09-23 Thread Jeff Garzik 

On 09/23/2010 03:55 AM, Jim Meyering wrote:

Better safe than sorry...
Unreported write failures can be unpleasant.
I fixed the one below so that a failure indication
can propagate up the call tree.  You might also want to
report the failure to stderr.

I let my editor automatically update the copyright date
and remove trailing spaces.
If you'd rather separate those from the fix,
let me know and I can adjust and resend.


Patch applied, thanks.

The typical preference is to receive whitespace and other cosmetic 
changes in a separate patch, thereby highlighting the functional changes.


But we're not so strict here that I would reject an otherwise useful 
patch...


Also FWIW, we're not very strict about reproducing the GCC-ish 
(GNU-ish?) style of "$FILENAME ($FUNCTION):" in each changelog -- though 
you're certainly welcome to continue, if that's your preference.


Given that "git show $COMMIT" will give you filename and per-diff-chunk 
function names, reproducing that in the git changelog entry seems 
somewhat redundant.  A simple, English-language summary of the change is 
fine.  Just a style tip, though, feel free to ignore!  :)


Jeff


--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH tabled] server/server.c (net_write_port): Don't ignore write error.

2010-09-23 Thread Jim Meyering 
Better safe than sorry...
Unreported write failures can be unpleasant.
I fixed the one below so that a failure indication
can propagate up the call tree.  You might also want to
report the failure to stderr.

I let my editor automatically update the copyright date
and remove trailing spaces.
If you'd rather separate those from the fix,
let me know and I can adjust and resend.

>From abe4be09fea8194fe0c4187c20ebaa45822c839c Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Thu, 23 Sep 2010 09:38:24 +0200
Subject: [PATCH tabled] server/server.c (net_write_port): Don't ignore write 
error.


Signed-off-by: Jim Meyering 
---
 server/server.c |   16 +---
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/server/server.c b/server/server.c
index 7a9fb7a..3398026 100644
--- a/server/server.c
+++ b/server/server.c
@@ -1,6 +1,6 @@

 /*
- * Copyright 2008-2009 Red Hat, Inc.
+ * Copyright 2008-2010 Red Hat, Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -1613,7 +1613,7 @@ void cld_update_cb(void)
  * server, the management of nodes is not in storage.c, which deals
  * with the interface to Chunk and little more.
  *
- * We don't even bother with registering this callback, just call it by name. 
+ * We don't even bother with registering this callback, just call it by name.
  *
  * The return value is used to re-arm storage rescan mechanism.
  */
@@ -1847,9 +1847,12 @@ static int net_write_port(const char *port_file,
   port_file, strerror(rc));
return -rc;
}
-   fprintf(portf, "%s:%s\n", tabled_srv.ourhost, port);
-   fclose(portf);
-   return 0;
+   if (fprintf(portf, "%s:%s\n", tabled_srv.ourhost, port) < 0) {
+   rc = errno;
+   fclose(portf);
+   return -rc;
+   }
+   return fclose(portf) ? -errno : 0;
 }

 /*
@@ -1959,7 +1962,7 @@ static int net_open_known(const char *portstr, bool 
is_status)
continue;

rc = net_open_socket(res->ai_family, res->ai_socktype,
-res->ai_protocol, 
+res->ai_protocol,
 res->ai_addrlen, res->ai_addr, is_status);
if (rc < 0)
goto err_out;
@@ -2348,4 +2351,3 @@ err_out:
closelog();
return rc;
 }
-
-- 
1.7.3.234.g7bba3

--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html