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

2010-09-24 Thread Jim Meyering 
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

2010-09-24 Thread Jim Meyering 
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

2010-09-24 Thread Jeff Garzik 

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:


---SNIP-
Please pull from 'upstream-linus' branch of
git://git.kernel.org/pub/scm/git/jgarzik/libata-dev.git upstream-linus

to receive the following updates:

 drivers/ata/ahci.c|  193 
+++--

 drivers/ata/libata-acpi.c |   40 +-
 drivers/ata/libata-core.c |3 +
 drivers/ata/libata.h  |2 +
 drivers/ata/pata_ali.c|2 +-
 include/linux/ata.h   |9 ++-
 include/linux/libata.h|   12 +++
 7 files changed, 178 insertions(+), 83 deletions(-)

Dirk Hohndel (1):
  pata_ali: trivial fix of a very frequent spelling mistake

Robert Hancock (1):
  ahci: display all AHCI 1.3 HBA capability flags (v2)

Tejun Heo (5):
  ahci: disable 64bit DMA by default on SB600s
  libata: cosmetic updates
  libata: implement more acpi filtering options
  libata: make gtf_filter per-dev
  ahci: filter FPDMA non-zero offset enable for Aspire 3810T

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index acd1162..4edca6e 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
[COMBINED PATCH FOLLOWS...]

---SNIP-
--
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-24 Thread Jim Meyering 
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 tabled 1/2] server/config.c: don't dereference NULL on OOM

2010-09-24 Thread Jeff Garzik 

On 09/24/2010 01:43 PM, Jim Meyering wrote:

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 Meyeringmeyer...@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


pulled from you  pushed upstream, 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