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 rpc/rpc.h -- 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 Garzikj...@garzik.org 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 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: [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 Meyeringmeyer...@redhat.com --- 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: 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 meyer...@redhat.com
---
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
[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 meyer...@redhat.com
---
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
[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 meyer...@redhat.com --- 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
[PATCH hail] lib/hstor.c: avoid an unconditional leak in append_qparam
Signed-off-by: Jim Meyering meyer...@redhat.com
---
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 j...@meyering.net 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
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 Meyeringmeyer...@redhat.com
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-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
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 meyer...@redhat.com\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
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
