[RFC v14][PATCH 04/54] General infrastructure for checkpoint restart

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl at cs.columbia.edu):
> ...
>> +static int checkpoint_write_header(struct ckpt_ctx *ctx)
>> +{
>> +	struct ckpt_hdr_header *h;
>> +	struct new_utsname *uts;
>> +	struct timeval ktv;
>> +	int ret;
>> +
>> +	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_HEADER);
> 
> ...
>> +	struct ckpt_hdr_tail *h;
>> +	int ret;
>> +
>> +	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_TAIL);
> 
> ...
>> +	struct ckpt_hdr_task *h;
>> +	int ret;
>> +
>> +	h = ckpt_hdr_get_type(ctx, sizeof(*h), CKPT_HDR_TASK);
> 
> ...
>> +/**
>> + * ckpt_hdr_get_type - get a hdr of certain size
>> + * @ctx: checkpoint context
>> + * @len: number of bytes to reserve
>> + *
>> + * Returns pointer to reserved space on hbuf
>> + */
>> +void *ckpt_hdr_get_type(struct ckpt_ctx *ctx, int len, int type)
>> +{
> 
> Observation (based on all callers in later patches as well): the second
> argument appears to be superfluous?  You should be able to determine
> based on type.

Not always: for instance, ckpt_hdr_file_xxxx all use CKPT_HDR_FILE,
but have different sizes (all share a common 'struct ckpt_hdr_file',
but actual payload differs).

(Besides, that would require adding a big table to decide the length
based on a type... which I don't really like).

Oren.

> 
> (The callers would look much friendlier without the 2nd arg imo)