[sheepdog] [PATCH] sheep: make epoch_log_read() clean

[Hitoshi Mitake]

At Mon, 18 Mar 2013 16:55:00 +0900,
MORITA Kazutaka wrote:
> 
> > diff --git a/include/internal_proto.h b/include/internal_proto.h
> > index f237a86..6b3866d 100644
> > --- a/include/internal_proto.h
> > +++ b/include/internal_proto.h
> > @@ -191,7 +191,7 @@ struct sd_node {
> >  
> >  struct epoch_log {
> >  	uint64_t ctime;
> > -	uint64_t time;
> > +	time_t time;
> 
> Can this guarantee that struct epoch_log is aligned to 64 bit
> boundaries?

It cannot guarantee the alignment. I'll fix in the next version.
BTW, where does the constraint of alignment come from?

> 
> > @@ -475,12 +481,16 @@ int epoch_log_read_remote(uint32_t epoch, struct sd_node *nodes, int len)
> >  		hdr.data_length = len;
> >  		hdr.obj.tgt_epoch = epoch;
> >  		hdr.epoch = sys_epoch();
> > -		ret = sheep_exec_req(&node->nid, &hdr, nodes);
> > +		ret = sheep_exec_req(&node->nid, &hdr, buf);
> >  		if (ret != SD_RES_SUCCESS)
> >  			continue;
> >  
> > +		nodes_len = rsp->data_length - sizeof(timestamp);
> > +		memcpy((void *)nodes, buf, nodes_len);
> > +		memcpy(timestamp, buf + nodes_len, sizeof(timestamp));
> > +
> 
> How about doing this copy only when timestamp is not NULL?  Then we
> can pass NULL to the 4th argument of epoch_log_read_remote.

It is a smarter way of dealing with timestamp. I'll do so in the next version.

> 
> 
> > -int epoch_log_read(uint32_t epoch, struct sd_node *nodes, int len)
> > +static int do_epoch_log_read(uint32_t epoch, struct sd_node *nodes, int len,
> > +			time_t *timestamp)
> >  {
> > -	int fd;
> > +	int fd, ret, nr_nodes;
> >  	char path[PATH_MAX];
> > +	struct stat epoch_stat;
> >  
> >  	snprintf(path, sizeof(path), "%s%08u", epoch_path, epoch);
> >  	fd = open(path, O_RDONLY);
> > -	if (fd < 0) {
> > -		sd_eprintf("failed to open epoch %"PRIu32" log", epoch);
> > -		return -1;
> > -	}
> > +	if (fd < 0)
> > +		goto err;
> >  
> > -	len = read(fd, nodes, len);
> > +	memset(&epoch_stat, 0, sizeof(epoch_stat));
> > +	ret = fstat(fd, &epoch_stat);
> > +	if (ret < 0)
> > +		goto err;
> >  
> > -	close(fd);
> > +	if (len < epoch_stat.st_size - sizeof(time_t))
> > +		goto err;
> >  
> > -	if (len < 0) {
> > -		sd_eprintf("failed to read epoch %"PRIu32" log", epoch);
> > -		return -1;
> > -	}
> > -	return len / sizeof(*nodes);
> > +	ret = xread(fd, nodes, epoch_stat.st_size - sizeof(timestamp));
> > +	if (ret < 0)
> > +		goto err;
> > +
> > +	assert(ret % sizeof(struct sd_node) == 0);
> > +	nr_nodes = ret / sizeof(struct sd_node);
> > +
> > +	ret = xread(fd, timestamp, sizeof(timestamp));
> > +	if (ret != sizeof(timestamp))
> > +		goto err;
> 
> Same here.  Actually, some callers of them don't need timestamp at
> all.
> 

I'll fix it like the above one.

Thanks for your coments,
Hitoshi