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

[MORITA Kazutaka]

> 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?

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


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

Thanks,

Kazutaka