[PATCH] don't expect inode name to be NUL-terminated (avoid read overrun)
* server/msg.c (msg_get): Copy only name_len bytes, then NUL-terminate, rather than using snprintf to copy up to and including nonexistent NUL. --- valgrind exposed this. The use of snprintf would have been correct if the inode name buffer (following the struct raw_inode) were NUL-terminated, but it is not. Invalid read of size 1 at 0x3502647FF7: vfprintf (vfprintf.c:1593) by 0x350266EFB1: vsnprintf (vsnprintf.c:120) by 0x350264F022: snprintf (snprintf.c:35) by 0x4061D5: msg_get (msg.c:451) by 0x407FD9: udp_rx_handle (server.c:164) by 0x408244: udp_rx (server.c:233) by 0x4091AA: udp_srv_event (server.c:640) by 0x409DE6: main_loop (server.c:1026) by 0x40A17E: main (server.c:1135) Address 0x4d2afae is 0 bytes after a block of size 62 alloc'd at 0x4A0515D: malloc (vg_replace_malloc.c:195) by 0x3505F35527: __os_umalloc (os_alloc.c:68) by 0x3505EF8F8D: __db_retcopy (db_ret.c:124) by 0x3505EF90EB: __db_ret (db_ret.c:69) by 0x3505ED93A7: __dbc_iget (db_cam.c:) by 0x3505EE5CB3: __db_get (db_iface.c:779) by 0x3505EE5FDA: __db_get_pp (db_iface.c:694) by 0x4040F6: cldb_inode_get (cldb.c:537) by 0x406069: msg_get (msg.c:430) by 0x407FD9: udp_rx_handle (server.c:164) by 0x408244: udp_rx (server.c:233) by 0x4091AA: udp_srv_event (server.c:640) by 0x409DE6: main_loop (server.c:1026) by 0x40A17E: main (server.c:1135) Here's a fix: server/msg.c |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/server/msg.c b/server/msg.c index f2dda59..8abb1e6 100644 --- a/server/msg.c +++ b/server/msg.c @@ -448,7 +448,8 @@ void msg_get(struct session *sess, const void *v) name_len = le32_to_cpu(inode->ino_len); inode_name = alloca(name_len + 1); - snprintf(inode_name, name_len + 1, "%s", (char *)(inode + 1)); + memcpy (inode_name, inode + 1, name_len); + inode_name[name_len] = 0; resp.inode_name = inode_name; resp.data.data_len = 0; @@ -1172,4 +1173,3 @@ err_out_noabort: sess_sendresp_generic(sess, resp_rc); free(h); } - -- 1.7.3.rc0.183.gb0497 -- 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] don't expect inode name to be NUL-terminated (avoid read overrun)
Jeff Garzik wrote: > On 09/10/2010 08:55 AM, Jim Meyering wrote: >> >> * server/msg.c (msg_get): Copy only name_len bytes, then NUL-terminate, >> rather than using snprintf to copy up to and including nonexistent NUL. >> --- >> >> valgrind exposed this. The use of snprintf would have been >> correct if the inode name buffer (following the struct raw_inode) >> were NUL-terminated, but it is not. > > applied -- good catch > > out of curiosity, what is your patch base? > > We combined cld and chunkd into a single 'hail' pkg, and from the > pathname, your patch was generated from the older cld pkg. We'd like > to find the source and replace cld/chunkd with 'hail'. > > F12? F13? rawhide? Hi Jeff, I was using the sources from here: git://git.kernel.org/pub/scm/daemon/cld/cld.git >From your comment there must be a hail git repository. Found it: http://git.kernel.org/?p=daemon/distsrv/hail.git;a=summary FYI, when I searched for hail's git repository initially, https://hail.wiki.kernel.org/ was inaccessible, so I found the above in a presumably-old cache. -- 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.
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
Re: [PATCH tabled] server/server.c (net_write_port): Don't ignore write error.
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
[PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
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
[PATCH tabled 2/2] server/server.c: don't deref NULL on failed strdup
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] server/server.c: don't deref NULL on failed strdup
>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] server/status.c: don't deref NULL on failed strdup
>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/bucket.c: don't deref NULL upon failed malloc
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/server.c don't deref NULL on failed malloc
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
Re: [tabled patch] abstract out TCP-write code
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 1/2] server/config.c: don't dereference NULL on OOM
Jeff Garzik wrote:
> 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);
Sure. Adjusted.
> 2) it is preferred to omit optional braces for singleton test+stmt
> style statements:
>
> + if (!pass) {
> + goto err_cmp;
> + }
Gladly. That's what I would have done in code I own, but there is a
braced single-line "else" block just above, so I presumed that the
style was "always use braces". (I think we have the same preferences,
since I too would use braces around the single-line "else" in that case,
though not if the "then" block had also been a one-liner.
> [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.
No problem.
> 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.
Will do.
You can tell that I'm too accustomed to posting FYI-patches
that I will shortly push -- or that I'll push upon review.
> 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.
Preaching to the choir ;-)
Thanks for spelling out your guidelines.
--
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
Jeff Garzik wrote:
> 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.
Here you go.
You can pull from the "oom" branch here:
git://git.infradead.org/users/meyering/tabled.git
I think I've addressed all of your preferences, merging
most OOM fixes into one commit, but not the two you mentioned
that should stay separate. I also left the sizeof(s) one separate.
However, I did leave the copyright year updates in.
If they're a problem, let me know and I'll do another round.
Otherwise, I can send a patch to update all of the
remaining ones to include 2010 so this won't be an
issue for 3 more months.
$ git shortlog HEAD ^origin/master
Jim Meyering (4):
server/server.c: use sizeof(s) rather than equivalent "64"
don't dereference NULL on OOM
server/status.c: don't deref NULL on failed strdup
server/bucket.c: don't deref NULL upon failed malloc
b/server/bucket.c | 25 -
b/server/config.c |7 +--
b/server/object.c |7 ++-
b/server/replica.c |7 +--
b/server/server.c |2 +-
b/server/status.c |8 +---
server/server.c| 13 +
7 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/server/bucket.c b/server/bucket.c
index eb03e03..cf42d2d 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,29 +788,36 @@ 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;
}
+#undef append_const
struct bucket_list_info {
char *prefix;
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",
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-
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
Jim Meyering wrote:
> Jeff Garzik wrote:
>
>> 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.
>
> Here you go.
> You can pull from the "oom" branch here:
> git://git.infradead.org/users/meyering/tabled.git
>
> I think I've addressed all of your preferences, merging
> most OOM fixes into one commit, but not the two you mentioned
> that should stay separate. I also left the sizeof(s) one separate.
>
> However, I did leave the copyright year updates in.
> If they're a problem, let me know and I'll do another round.
> Otherwise, I can send a patch to update all of the
> remaining ones to include 2010 so this won't be an
> issue for 3 more months.
>
> $ git shortlog HEAD ^origin/master
> Jim Meyering (4):
> server/server.c: use sizeof(s) rather than equivalent "64"
> don't dereference NULL on OOM
> server/status.c: don't deref NULL on failed strdup
> server/bucket.c: don't deref NULL upon failed malloc
>
> b/server/bucket.c | 25 -
> b/server/config.c |7 +--
> b/server/object.c |7 ++-
> b/server/replica.c |7 +--
> b/server/server.c |2 +-
> b/server/status.c |8 +---
> server/server.c| 13 +
> 7 files changed, 47 insertions(+), 22 deletions(-)
That wasn't quite right.
Here's an updated patch, including e.g., this:
diff --git a/server/config.c b/server/config.c
index a58a0e6..9539cfd 100644
--- a/server/config.c
+++ b/server/config.c
@@ -436,7 +436,8 @@ void read_config(void)
memset(&ctx, 0, sizeof(struct config_context));
- if (!(tabled_srv.port = strdup("8080"))) {
+ tabled_srv.port = strdup("8080");
+ if (!tabled_srv.port) {
applog(LOG_ERR, "no core");
exit(1);
}
==
diff --git a/server/bucket.c b/server/bucket.c
index eb03e03..cf42d2d 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,29 +788,36 @@ 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;
}
+#undef append_const
struct bucket_list_info {
char *prefix;
diff --git a/server/config.c b/server/config.c
index f94886e..dad6579 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
@@ -437,6 +437,10 @@ void read_config(void)
memset(&ctx, 0, sizeof(struct config_context));
Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
Jeff Garzik wrote: > On 09/24/2010 07:32 AM, Jim Meyering wrote: >> You can pull from the "oom" branch here: >>git://git.infradead.org/users/meyering/tabled.git > > Got nearly everything perfect. Need one more minor yet important > change. As described in doc/contributions.txt, every changeset MUST > have a Signed-off-by line at the end of a changeset's description. > > I was able to pull and build just fine, so your git repo setup and > push appears correct. > > Also, in your pull request, please put the branch immediately > following the repo URL on the same line, for easier cut-n-paste. > Here's how Linus requests his pull-requests to look: Ok. I've added those pesky S.O.B lines with this: git filter-branch --msg-filter \ 'cat && printf "\nSigned-off-by: Jim Meyering \n"' \ HEAD~4..HEAD and pushed the result. Please pull from the 'oom' branch of git://git.infradead.org/users/meyering/tabled.git I presume there's no need to re-post the diffstat or diffs. -- 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 hail] lib/hstor.c: avoid an unconditional leak in append_qparam
Signed-off-by: Jim Meyering
---
I would have preferred to insert a single line right before the
huri_field_escape call:
char *v = strdup(val);
[would result in a more compact, single-hunk patch]
but it looks like hail uses the anachronistic (pre-C99)
"declare all vars at outer scope" style, so I conformed.
lib/hstor.c |5 -
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/lib/hstor.c b/lib/hstor.c
index 6c67bfa..79e0420 100644
--- a/lib/hstor.c
+++ b/lib/hstor.c
@@ -676,6 +676,7 @@ static GString *append_qparam(GString *str, const char
*key, const char *val,
char *arg_char)
{
char *stmp;
+ char *v;
str = g_string_append(str, arg_char);
arg_char[0] = '&';
@@ -683,9 +684,11 @@ static GString *append_qparam(GString *str, const char
*key, const char *val,
str = g_string_append(str, key);
str = g_string_append(str, "=");
- stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK);
+ v = strdup(val);
+ stmp = huri_field_escape(v, QUERY_ESCAPE_MASK);
str = g_string_append(str, stmp);
free(stmp);
+ free(v);
return str;
}
--
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 hail] lib/hstor.c: avoid an unconditional leak in append_qparam
Pete Zaitcev wrote: > On Mon, 27 Sep 2010 10:53:06 +0200 > Jim Meyering wrote: > >> -stmp = huri_field_escape(strdup(val), QUERY_ESCAPE_MASK); >> +v = strdup(val); >> +stmp = huri_field_escape(v, QUERY_ESCAPE_MASK); >> str = g_string_append(str, stmp); >> free(stmp); >> +free(v); > > I think you may be fooled by the ridiculous calling convention > of huri_field_escape(). It takes a pointer to heap, then either > returns its argument, or reallocates it, frees the argument, and > returns the reallocated area. It frees with g_free, so it assumes > its equivalence with free(), haha. > > The end result, it either returns what strdup returned of frees it. > Therefore if you free what strudup returned, you double-free it. Oh! You're right. I missed the "g_free (signed_str);" at the end of huri_field_escape. Sorry about that. > I honestly think this madness must stop and huri_field_escape > must allocate a new buffer every time. Then we would not need > the strdup there at all. It only exists to satisfy the requirement > to pass a pointer to heap in case val is a const or whatnot. Making a function like huri_field_escape free a buffer allocated by the caller does seem to violate something fundamental. -- 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 hail] chunkd: don't leak an FS object iterator
chk_list_objs called fs_list_objs_open without also calling fs_list_objs_close. 32,808 bytes in 1 blocks are definitely lost in loss record 413 of 419 at 0x4A0515D: malloc (vg_replace_malloc.c:195) by 0x31BA8A26D0: __alloc_dir (opendir.c:184) by 0x405619: fs_list_objs_open (be-fs.c:974) by 0x40B202: chk_list_objs (selfcheck.c:41) by 0x40B575: chk_dbscan (selfcheck.c:131) by 0x40B628: chk_thread_scan (selfcheck.c:147) by 0x40B757: chk_thread_command (selfcheck.c:179) by 0x40B890: chk_thread_func (selfcheck.c:219) by 0x31BC464E83: g_thread_create_proxy (gthread.c:1893) by 0x31BB407760: start_thread (pthread_create.c:301) by 0x31BA8E151C: clone (clone.S:115) Signed-off-by: Jim Meyering --- Thanks to Pete for catching my error. To make up for that, here's a real leak fix: chunkd/selfcheck.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/chunkd/selfcheck.c b/chunkd/selfcheck.c index f3713da..86d3eb2 100644 --- a/chunkd/selfcheck.c +++ b/chunkd/selfcheck.c @@ -100,6 +100,7 @@ static void chk_list_objs(struct chk_tls *tls, uint32_t table_id) free(fn); } + fs_list_objs_close(&lister); } static void chk_dbscan(struct chk_tls *tls) -- 1.7.3.293.gca9a76 -- 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 hail] chunkd: don't leak an FS object iterator
Jeff Garzik wrote: > On 09/29/2010 11:20 AM, Jim Meyering wrote: >> >> chk_list_objs called fs_list_objs_open without also calling >> fs_list_objs_close. >> >> 32,808 bytes in 1 blocks are definitely lost in loss record 413 of 419 >> at 0x4A0515D: malloc (vg_replace_malloc.c:195) >> by 0x31BA8A26D0: __alloc_dir (opendir.c:184) >> by 0x405619: fs_list_objs_open (be-fs.c:974) >> by 0x40B202: chk_list_objs (selfcheck.c:41) >> by 0x40B575: chk_dbscan (selfcheck.c:131) >> by 0x40B628: chk_thread_scan (selfcheck.c:147) >> by 0x40B757: chk_thread_command (selfcheck.c:179) >> by 0x40B890: chk_thread_func (selfcheck.c:219) >> by 0x31BC464E83: g_thread_create_proxy (gthread.c:1893) >> by 0x31BB407760: start_thread (pthread_create.c:301) >> by 0x31BA8E151C: clone (clone.S:115) > > After seeing a few valgrind references from you, I'm curious... do you > by chance happen to have a valgrind suppression file for openssl on > Fedora? No, I ignored them. But to create a so-called "suppressions" file, you can invoke valgrind with --gen-suppressions=all. You select the ssl-related entries and put them in a file, say F, then whenever you run valgrind, use --suppressions=F to make it ignore those. > I've been wanting to run valgrind on chunkd, but each time I attempt > it, I -- and valgrind -- have been overwhelmed by openssl false > positives. openssl, deep in its RAND_xxx functions, intentionally does > crazy stuff like using random, uninitialized stack contents as RNG > entropy. Cute, but valgrind quite rightly complains loudly about it. > > It's a topic I've been meaning to research, because I currently lack > the valgrind-fu necessary to have an effective valgrind+chunkd > session. BTW, Here are a few more that I haven't investigated: ==4506== 1 errors in context 1 of 7: ==4506== Syscall param pwrite64(buf) points to uninitialised byte(s) ==4506==at 0x31BB40EDE3: __pwrite_nocancel (syscall-template.S:82) ==4506==by 0x31BE137AD0: __os_io (os_rw.c:92) ==4506==by 0x31BE125733: __memp_pgwrite (mp_bh.c:394) ==4506==by 0x31BE125954: __memp_bhwrite (mp_bh.c:168) ==4506==by 0x31BE134482: __memp_sync_int (mp_sync.c:530) ==4506==by 0x31BE0CBD66: __db_sync (db_am.c:706) ==4506==by 0x31BE0CA652: __db_refresh (db.c:820) ==4506==by 0x31BE0CA8C0: __db_close (db.c:695) ==4506==by 0x31BE0E64A7: __db_close_pp (db_iface.c:253) ==4506==by 0x403450: cldb_down (cldb.c:414) ==4506==by 0x409A58: main (server.c:1143) ==4506== Address 0x4f005e3 is 4,083 bytes inside a block of size 4,096 alloc'd ==4506==at 0x4A0515D: malloc (vg_replace_malloc.c:195) ==4506==by 0x31BE135237: __os_malloc (os_alloc.c:253) ==4506==by 0x31BE1257FC: __memp_pgwrite (mp_bh.c:385) ==4506==by 0x31BE125954: __memp_bhwrite (mp_bh.c:168) ==4506==by 0x31BE134482: __memp_sync_int (mp_sync.c:530) ==4506==by 0x31BE0CBD66: __db_sync (db_am.c:706) ==4506==by 0x31BE0CA652: __db_refresh (db.c:820) ==4506==by 0x31BE0CA8C0: __db_close (db.c:695) ==4506==by 0x31BE0E64A7: __db_close_pp (db_iface.c:253) ==4506==by 0x403450: cldb_down (cldb.c:414) ==4506==by 0x409A58: main (server.c:1143) ==4506== Uninitialised value was created by a stack allocation ==4506==at 0x404878: cldb_lock_add (cldb.c:967) ==4506== ==4506== ==4506== 1 errors in context 2 of 7: ==4506== Syscall param pwrite64(buf) points to uninitialised byte(s) ==4506==at 0x31BB40EDE3: __pwrite_nocancel (syscall-template.S:82) ==4506==by 0x31BE137AD0: __os_io (os_rw.c:92) ==4506==by 0x31BE1207C4: __log_write (log_put.c:1227) ==4506==by 0x31BE121477: __log_flush_int (log_put.c:1010) ==4506==by 0x31BE122CDC: __log_put (log_put.c:496) ==4506==by 0x31BE146077: __txn_regop_log (txn_auto.c:234) ==4506==by 0x31BE142A32: __txn_commit (txn.c:656) ==4506==by 0x31BE142ABE: __txn_commit_pp (txn.c:517) ==4506==by 0x4073A1: msg_lock (msg.c:1150) ==4506==by 0x407869: udp_rx_handle (server.c:164) ==4506==by 0x407BF3: udp_rx (server.c:265) ==4506==by 0x408A3A: udp_srv_event (server.c:640) ==4506==by 0x409676: main_loop (server.c:1026) ==4506==by 0x409A0E: main (server.c:1135) ==4506== Address 0x52d5380 is not stack'd, malloc'd or (recently) free'd ==4506== Uninitialised value was created by a stack allocation ==4506==at 0x404878: cldb_lock_add (cldb.c:967) ==4506== ==4506== ==4506== 1 errors in context 3 of 7: ==4506== Conditional jump or move depends on uninitialised value(s) ==4506==at 0x31BE121A78: __log_putr (log_put.c:732) ==4506==by 0x31BE1228B8: __log_put (log_put.c:464) ==4506==by 0x31BE064E28: __ham_insdel_log (hash_auto.c:250) ==4506==by 0x31BE06E86F: __ham_add_el (hash_page.c:2197) ==4506==by 0x31BE05E5EC: _
[PATCH hail] const-correctness tweaks
Make write_cb callback's buffer parameter const, like all write-like
functions.
Give a few "char *" parameters the "const" attribute.
Signed-off-by: Jim Meyering
---
It looks like most of hail's interfaces are const-correct,
but one stood out because it provokes a warning when I tried to
pass a const-correct write_cb function to hstor_get from iwhd:
proxy.c:382: warning: passing argument 4 of 'hstor_get' from \
incompatible pointer type
/usr/include/hstor.h:173: note: expected \
'size_t (*)(void *, size_t, size_t, void *)' but argument is of type \
'size_t (*)(const void *, size_t, size_t, void *)'
In case you feel comfortable fixing this, here's a patch:
include/hstor.h |4 ++--
lib/hstor.c |5 +++--
lib/hutil.c |2 +-
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/include/hstor.h b/include/hstor.h
index 8620d3b..b47387b 100644
--- a/include/hstor.h
+++ b/include/hstor.h
@@ -132,7 +132,7 @@ enum ReqACLC {
/* hutil.c */
extern char *hutil_time2str(char *buf, int len, time_t time);
extern time_t hutil_str2time(const char *timestr);
-extern int hreq_hdr_push(struct http_req *req, char *key, char *val);
+extern int hreq_hdr_push(struct http_req *req, const char *key, const char
*val);
extern char *hreq_hdr(struct http_req *req, const char *key);
extern void hreq_sign(struct http_req *req, const char *bucket, const char
*key,
char *b64hmac_out);
@@ -171,7 +171,7 @@ extern bool hstor_del_bucket(struct hstor_client *hstor,
const char *name);
extern struct hstor_blist *hstor_list_buckets(struct hstor_client *hstor);
extern bool hstor_get(struct hstor_client *hstor, const char *bucket, const
char *key,
-size_t (*write_cb)(void *, size_t, size_t, void *),
+size_t (*write_cb)(const void *, size_t, size_t, void *),
void *user_data, bool want_headers);
extern void *hstor_get_inline(struct hstor_client *hstor, const char *bucket,
const char *key, bool want_headers, size_t *len);
diff --git a/lib/hstor.c b/lib/hstor.c
index d0d87c7..7f638ec 100644
--- a/lib/hstor.c
+++ b/lib/hstor.c
@@ -86,7 +86,8 @@ err_out:
return NULL;
}
-static size_t all_data_cb(void *ptr, size_t size, size_t nmemb, void
*user_data)
+static size_t all_data_cb(const void *ptr, size_t size, size_t nmemb,
+ void *user_data)
{
GByteArray *all_data = user_data;
int len = size * nmemb;
@@ -378,7 +379,7 @@ bool hstor_del_bucket(struct hstor_client *hstor, const
char *name)
}
bool hstor_get(struct hstor_client *hstor, const char *bucket, const char *key,
-size_t (*write_cb)(void *, size_t, size_t, void *),
+size_t (*write_cb)(const void *, size_t, size_t, void *),
void *user_data, bool want_headers)
{
struct http_req req;
diff --git a/lib/hutil.c b/lib/hutil.c
index 7439d52..13a8d5e 100644
--- a/lib/hutil.c
+++ b/lib/hutil.c
@@ -131,7 +131,7 @@ static void cust_fin(struct custom_hdr_vec *cv)
/*
*/
-int hreq_hdr_push(struct http_req *req, char *key, char *val)
+int hreq_hdr_push(struct http_req *req, const char *key, const char *val)
{
struct http_hdr *hdr;
--
1.7.3.1.50.g1e633
--
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 hail] const-correctness tweaks
Jeff Garzik wrote: > On 10/06/2010 08:07 AM, Jim Meyering wrote: ... >> It looks like most of hail's interfaces are const-correct, >> but one stood out because it provokes a warning when I tried to >> pass a const-correct write_cb function to hstor_get from iwhd: ... >> include/hstor.h |4 ++-- >> lib/hstor.c |5 +++-- >> lib/hutil.c |2 +- >> 3 files changed, 6 insertions(+), 5 deletions(-) > > This requires updating test/large-object.c in tabled, too. Would you > mind sending along that companion patch? Sure. Posting separately. With only one use, the tendrils were relatively short. -- 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] adapt to changed signature of hstor_get's callback function
* test/large-object.c: Hail has changed hstor_get's callback function
so that it now declares its buffer to be "const", as all write-like
functions do. Adjust this file's hstor_get callback parameter and
propagate that, as required, to the local functions it uses to operate
on that now-read-only buffer.
Signed-off-by: Jim Meyering
---
test/large-object.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/test/large-object.c b/test/large-object.c
index dbe2027..fc7d03c 100644
--- a/test/large-object.c
+++ b/test/large-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
@@ -60,7 +60,7 @@ static char key[] = "Key of Large Object";
#define CSUM_INIT 0x
-static void incrsum(unsigned int *psum, unsigned char *data, size_t len)
+static void incrsum(unsigned int *psum, const unsigned char *data, size_t len)
{
unsigned int sum;
@@ -108,7 +108,7 @@ static size_t put_cb(void *ptr, size_t membsize, size_t
nmemb, void *user_data)
return rem;
}
-static size_t get_one(struct get_ctx *ctx, unsigned char *data, size_t len)
+static size_t get_one(struct get_ctx *ctx, const unsigned char *data, size_t
len)
{
unsigned num;
size_t rem;
@@ -143,7 +143,8 @@ static size_t get_one(struct get_ctx *ctx, unsigned char
*data, size_t len)
return rem;
}
-static size_t get_cb(void *ptr, size_t membsize, size_t nmemb, void *user_data)
+static size_t get_cb(const void *ptr, size_t membsize, size_t nmemb,
+void *user_data)
{
struct get_ctx *ctx = user_data;
size_t togo, len;
--
1.7.3.1.50.g1e633
--
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 hail] const-correctness tweaks
Jeff Garzik wrote: > On 10/06/2010 08:07 AM, Jim Meyering wrote: >> >> Make write_cb callback's buffer parameter const, like all write-like >> functions. >> Give a few "char *" parameters the "const" attribute. >> >> Signed-off-by: Jim Meyering >> --- >> >> It looks like most of hail's interfaces are const-correct, >> but one stood out because it provokes a warning when I tried to >> pass a const-correct write_cb function to hstor_get from iwhd: >> >> proxy.c:382: warning: passing argument 4 of 'hstor_get' from \ >>incompatible pointer type >> /usr/include/hstor.h:173: note: expected \ >>'size_t (*)(void *, size_t, size_t, void *)' but argument is of type >> \ >>'size_t (*)(const void *, size_t, size_t, void *)' >> >> In case you feel comfortable fixing this, here's a patch: > > > Sorry for not getting back to this; I had hoped to solve some > additional problems that cropped up, but didn't have time. So, to > forestall further delay, > > libtool: compile: gcc -DHAVE_CONFIG_H -I. -I.. -I../include -pthread > -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include > -I/usr/include/libxml2 -O2 -Wall -Wshadow -g -MT hutil.lo -MD -MP -MF > .deps/hutil.Tpo -c hutil.c -o hutil.o > hutil.c: In function ‘hreq_hdr_push’: > hutil.c:145: warning: assignment discards qualifiers from pointer > target type > hutil.c:146: warning: assignment discards qualifiers from pointer > target type > > warnings appear after this patch. When solving these warnings with > const' markers, it quickly becomes a bit of a rat's nest. > > At a minimum, the write_cb callback signature must match libcurl's, > which does not use 'const'. I can see this makes sense from libcurl > implementation's perspective, even if it does not really match the > constness one expects from a foo-get function. Hi Jeff. Sorry I didn't notice that the first time. I built with ./autogen.sh && ./configure && make. It looks like you recommend -Wall -Wshadow. The two warnings above are the only ones I see with the patch, and they're easy to fix. When storing const pointer params into a struct like that, I've found that it's best to cast away the "const", which really does reflect the semantics: by using "const" on the parameter, I view it as promising not to deref through the pointer *in that function*. Since it's usually not reasonable to make the struct member "const" (as you saw, it propagates too far and often ends up being contradictory), the lesser evil of the cast is preferable here. If you're still game, the following incremental patch seems to be enough for me: Let me know and I'll resubmit the full one. diff --git a/lib/hutil.c b/lib/hutil.c index 13a8d5e..b74460b 100644 --- a/lib/hutil.c +++ b/lib/hutil.c @@ -142,8 +142,8 @@ int hreq_hdr_push(struct http_req *req, const char *key, const char *val) val++; hdr = &req->hdr[req->n_hdr++]; - hdr->key = key; - hdr->val = val; + hdr->key = (char *) key; + hdr->val = (char *) val; return 0; } -- 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 hail] const-correctness tweaks
Jeff Garzik wrote: ... >> Hi Jeff. >> >> Sorry I didn't notice that the first time. >> I built with ./autogen.sh&& ./configure&& make. >> It looks like you recommend -Wall -Wshadow. >> >> The two warnings above are the only ones I see with the patch, >> and they're easy to fix. When storing const pointer params into >> a struct like that, I've found that it's best to cast away the "const", >> which really does reflect the semantics: by using "const" on the >> parameter, I view it as promising not to deref through the pointer >> *in that function*. Since it's usually not reasonable to make >> the struct member "const" (as you saw, it propagates too far >> and often ends up being contradictory), the lesser evil of the cast >> is preferable here. >> >> If you're still game, the following incremental patch seems to be >> enough for me: Let me know and I'll resubmit the full one. > > Well, my primary concern now originates from curl_easy_setopt(3) > documentation: > >CURLOPT_WRITEFUNCTION > Function pointer that should match the following > prototype: size_t function( void *ptr, size_t size, > size_t nmemb, void *stream); > > hstor's callback is passed directly to libcurl, so we seem to be bound > by outside constraints, no? I compiled hail (with that patch) on F13 with -Wall -Wshadow with no warnings. That curl_easy_setopt documentation seems to be overly strict, or perhaps out of date?. When I compare with the code (curl/typecheck-gcc.h), I see all of the necessary "const" attributes: /* evaluates to true if expr is of type curl_write_callback or "similar" */ #define _curl_is_write_cb(expr) \ (_curl_is_read_cb(expr) ||\ __builtin_types_compatible_p(__typeof__(expr), __typeof__(fwrite)) || \ __builtin_types_compatible_p(__typeof__(expr), curl_write_callback) || \ _curl_callback_compatible((expr), _curl_write_callback1) ||\ _curl_callback_compatible((expr), _curl_write_callback2) ||\ _curl_callback_compatible((expr), _curl_write_callback3) ||\ _curl_callback_compatible((expr), _curl_write_callback4) ||\ _curl_callback_compatible((expr), _curl_write_callback5) ||\ _curl_callback_compatible((expr), _curl_write_callback6)) typedef size_t (_curl_write_callback1)(const char *, size_t, size_t, void*); typedef size_t (_curl_write_callback2)(const char *, size_t, size_t, const void*); typedef size_t (_curl_write_callback3)(const char *, size_t, size_t, FILE*); typedef size_t (_curl_write_callback4)(const void *, size_t, size_t, void*); typedef size_t (_curl_write_callback5)(const void *, size_t, size_t, const void*); typedef size_t (_curl_write_callback6)(const void *, size_t, size_t, FILE*); But even if curl were requiring some suboptimal signature, it would be nice not to impose that on all projects that use hail. Are there older curl headers that do require the const-free signature? If there are and you want to support them, too, let me know -- maybe I can cook up an autoconf test to make things work there, with minimal impact. -- 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 hail] const-correctness tweaks
Jeff Garzik wrote: ... >> But even if curl were requiring some suboptimal signature, >> it would be nice not to impose that on all projects that use hail. >> Are there older curl headers that do require the const-free signature? >> If there are and you want to support them, too, let me know -- maybe >> I can cook up an autoconf test to make things work there, with minimal >> impact. > > Nah, I wouldn't worry about the const signature, it's probably just > out of date documentation. If users appear running old OS's or OS > versions, we can tackle autoconf'ing on a piecemeal basis as needs > arise. > > Committed these patches of yours to hail.git and tabled.git. 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: AC_CONFIG_MACRO_DIR([m4])
Pete Zaitcev wrote: > Hi, Jim: > > Autoconf printed a warning when reconfiguting Hail, so I gave up and > added this: > > diff --git a/configure.ac b/configure.ac > index 9cfad23..d378854 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -62,6 +62,8 @@ AC_PROG_GCC_TRADITIONAL > AM_PROG_CC_C_O > AM_PROG_LIBTOOL > > +AC_CONFIG_MACRO_DIR([m4]) > + > dnl Checks for header files. > AC_HEADER_STDC > dnl AC_CHECK_HEADERS(sys/ioctl.h unistd.h) > > Now I have a directory m4/ with symlinks... This does not seem to be > helping any portability, unless I miss where the promised macro are > being saved locally. What was this about, do you happen to know? Hi Pete, The symlinks are ok, since "make dist" dereferences them when creating a tarball. However, if for some reason you find a problem due to the use of symlinks (in that case, please let us know -- who knows, might have to change the default) you can add --copy (-c) to the libtoolize invocation in autogen.sh. You'll also want the following patch, so that aclocal knows where to find the .m4 files: diff --git a/Makefile.am b/Makefile.am index 38a1d92..e5bf438 100644 --- a/Makefile.am +++ b/Makefile.am @@ -2,7 +2,8 @@ ## Toplevel Makefile.am ## +ACLOCAL_AMFLAGS = -I m4 + SUBDIRS= doc lib include cld chunkd tools test EXTRA_DIST = autogen.sh Doxyfile COPYING LICENSE - -- 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: AC_CONFIG_MACRO_DIR([m4])
Jeff Garzik wrote: > On 12/06/2010 12:44 PM, Pete Zaitcev wrote: >> On Mon, 06 Dec 2010 12:32:22 -0500 >> Jeff Garzik wrote: >> >>> Keeping the "correct libtool macros in-tree" implies adding a pointless >>> maintenance burden. The distro always gives us correct, up-to-date >>> files. Why would hail want to potentially lag upstream's version of >>> these macros, forcing us to manually track macros that are currently >>> updated automatically for each ./autogen.sh invocation? >> >> I presumed that the important part is a compatibility between the >> syntax used in various .am files and the libtool scriptography that >> underpins them. "Lagging" upstream has no downside in this case >> (unlike zlib, where security fixes may exist). > > It does not seem optimal to run a current libtool with outdated macro > files. In all cases except current one, you're checking in third > party, maintained, versioned files to hail.git where they will be > less-well maintained, and generally out-of-date vis a vis current > [upstream | Fedora]. Hi Jeff, The patch that adds those two lines does not (and IMHO should not) imply that a project would version-control those files. Typically, those symlinks exist only in your working directory, and not in any project's VC repository. If you have no other files in m4/, you can simply .gitignore the entire m4/ directory. > Where is the value in performing this additional work, besides > silencing a warning seen only by git repo users? Yeah, either way it's not a big deal. -- 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 2/3] CLD: switch network proto from UDP to TCP
Pete Zaitcev wrote: > On Fri, 31 Dec 2010 05:57:28 -0500 > Jeff Garzik wrote: > >> +struct cldc_tcp *tcp = private; >> +ssize_t rc; >> +struct ubbp_header ubbp; >> + >> +memcpy(ubbp.magic, "CLD1", 4); >> +ubbp.op_size = (buflen << 8) | 1; >> +#ifdef WORDS_BIGENDIAN >> +swab32(ubbp.op_size); >> +#endif >> + >> +rc = write(tcp->fd, &ubbp, sizeof(ubbp)); > > Why not this: > > unsigned int n; > > n = (buflen << 8) | 1; > ubbp.op_size = GUINT32_TO_LE(n); Nice. Avoiding those pesky in-function #ifdefs makes the code more readable. IMHO, this is kinder still on the eyes of reviewers, since the types of "n" and "rc" stay even closer to each definition/first-use: unsigned int n = (buflen << 8) | 1; ubbp.op_size = GUINT32_TO_LE(n); ssize_t rc = write(tcp->fd, &ubbp, sizeof(ubbp)); But if your coding guidelines require the all-vars-decl'd-at-top style (seriously anachronistic and more prone to merge conflicts), then I guess that's not an option... Though you may want to reconsider the policy, if your goal is portability: For the record, compilers that reject decl-after-stmt are not relevant any more. At least, there are so few that I've been able to keep that sort of construct in coreutils for the past several years. I used to maintain a c99-to-c89 patch, but removed even that almost two years ago. -- 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
FYI, rpc/ is gone from Fedora 15
FYI, /usr/include/rpc/ no longer exists, as of F15's glibc-headers-2.13.90-10, so hail's lib/cld_msg_rpc.h will have to do something about this #include: $ grep rpc.h lib/cld_msg_rpc.h #include -- 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
