Re: [PATCH 5/5][lxc] Hook up lxc_checkpoint() with app_checkpoint()

[Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>]

Daniel Lezcano [dlezcano-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org] wrote:
>> +find_cinit_pid(const char *name)
>>  {
>> +	struct lxc_command command = {
>> +		.request = { .type = LXC_COMMAND_CINIT_PID },
>> +	};
>> +
>> +	int ret, stopped;
>> +
>> +	ret = lxc_command(name, &command, &stopped);
>> +	if (ret < 0) {
>> +		ERROR("failed to send command");
>> +		return -1;
>> +	}
>> +
>> +	ERROR("find_cinit_pid %d\n", command.answer.ret);
>> +
>> +	return command.answer.ret;
>> +}
>
> I just committed the same function for the lxc_attach command. I think  
> you can reuse it.

Ok, will try to reuse it.

>
>> +int lxc_checkpoint(const char *name, const char *statefile, int lxc_flags)
>> +{
>> +	int ret;
>> +	int pid;
>> +	int flags;
>> +	struct stat statbuf;
>> +	struct app_checkpoint_args crargs;
>> +
>> +	if (access(statefile, F_OK) == 0) {
>> +		ret = stat(statefile, &statbuf);
>> +		if (ret < 0) {
>> +			ERROR("stat(%s): %s\n", statefile, strerror(errno));
>> +			return -1;
>> +		}
>> +
>> +		if (S_ISDIR(statbuf.st_mode)) {
>> +			ERROR("--directory option not implemented");
>> +			return -1;
>> +		} else {
>> +			ERROR("Checkpoint image file %s exists\n", statefile);
>> +			return -1;
>> +		}
>> +	}
>
> For the checkpoint, you don't need to check if it's a directory or a  
> file (but it should be done at restart time).

I need to open() the statefile and pass its fd to app_checkpoint().
If I don't check for directory, the open() below would fail, only
because of the O_RDWR flag. By checking, we could give a more useful
error message ?

> I am not sure 'access' is  
> really necessary because the O_EXCL is set in the open below.

Agree. How about just the stat() and the error message ?

Maybe later we could bury this code under say #ifdef USERCR so
if LXC is configured without USERCR, the current behavior is preserved ?

>
>> +
>> +	pid = find_cinit_pid(name);
>> +	if (pid < 0) {
>> +		ERROR("Unable to find cinit pid");
>> +		return -1;
>> +	}
>> +
>> +	memset(&crargs, 0, sizeof(crargs));
>> +
>> +	ret = open(statefile, O_CREAT|O_RDWR|O_EXCL, 0644);
>> +	if (ret < 0) {
>> +		ERROR("open(%s) failed\n", statefile);
>> +		return -1;
>> +	}
>
> As the statefile may contain sensible data, it would be preferable to  
> set it 0600, no ?

Agree.