[PATCH] Replication patches, second try

Jeff Darcy jdarcy at redhat.com
Wed Oct 6 12:27:25 UTC 2010


On 10/05/2010 05:51 PM, Jim Meyering wrote:
> Jeff Darcy wrote:
>> This includes a new control function that the replication tests use to
>> check that there are no replicated operations still pending before it
>> checks results.  It also includes the fix for the segfault noted in my
>> previous email.
> ...
> 
> Overall this is all fine.
> I've suggested a couple minor improvements.
> 
>> Subject: [PATCH 1/2] Add rep_count control operation on API root.
>>
>> This also fixes the segfault from the previous pthread-related commit.
> 
> The segfault-fixing part no longer applies.
> 
>>  proxy.c |   13 ++++++++++-
>>  proxy.h |    1 +
>>  rest.c  |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 85 insertions(+), 1 deletions(-)
>>
>> diff --git a/proxy.c b/proxy.c
>> index fa25e54..59bdf66 100644
>> --- a/proxy.c
>> +++ b/proxy.c
>> @@ -102,6 +102,7 @@ static repl_item	*queue_tail	= NULL;
>>  static pthread_mutex_t	 queue_lock;
>>  static sem_t		 queue_sema;
>>  static json_t		*config		= NULL;
>> +static int		 rep_count	= 0;
> 
> It'd be nice to make that an unsigned type.

It should be a gint, since that's what the glib routines actually use.

> Then we don't have to worry what happens if it ever goes negative.

I'd rather see negative values than underflows, and since the glib type
is signed that's what we'd get if we accidentally skipped an increment
but left the decrement.  Both negative and very large positive values
would indicate errors, but of different types that it might be helpful
to distinguish.

> 
> ...
>> +int
> 
> Match the new type of rep_count, of course.
> 
>> +get_rep_count (void)
>> +{
>> +	return g_atomic_int_get(&rep_count);
>> +}
>> diff --git a/proxy.h b/proxy.h
> ...
>> diff --git a/rest.c b/rest.c
>> index f501674..17dad68 100644
>> --- a/rest.c
>> +++ b/rest.c
>> @@ -1132,6 +1132,76 @@ create_bucket (char *name)
>>  }
>>
>>  static int
>> +control_api_root (void *cctx, struct MHD_Connection *conn, const char *url,
>> +		  const char *method, const char *version, const char *data,
>> +		  size_t *data_size, void **rctx)
>> +{
>> +	struct MHD_Response	*resp;
>> +	my_state		*ms	= *rctx;
>> +	int			 rc;
>> +	char			*op;
>> +	char			 buf[80];
>> +	int			 len;
>> +
>> +	(void)cctx;
>> +	(void)method;
>> +	(void)version;
>> +
>> +	DPRINTF("ROOT POST (%s, %zu)\n",url,*data_size);
>> +
>> +	if (ms->state == MS_NEW) {
>> +		ms->state = MS_NORMAL;
>> +		ms->url = (char *)url;
>> +		ms->dict = g_hash_table_new_full(
>> +			g_str_hash,g_str_equal,free,free);
>> +		ms->cleanup |= CLEANUP_DICT;
>> +		ms->post = MHD_create_post_processor(conn,4096,
> 
> If you would give this constant a name,
> then your lazy reviewer might be able to get
> away without reading MHD_create_post_processor documentation/code.
> 
>> +			post_iterator,ms->dict);
>> +		ms->cleanup |= CLEANUP_POST;
>> +		return MHD_YES;
>> +	}
>> +
>> +	if (*data_size) {
> ...
> 
>> Subject: [PATCH 2/2] Add replication tests.
> 
> Thanks!


More information about the iwhd-devel mailing list