[CRIU] [PATCH]v2 crtools: write pidfile, when service/page server is run as daemon and "--pidfile" is set

[Pavel Emelyanov]

On 09/16/2013 01:56 AM, Cyrill Gorcunov wrote:
> On Mon, Sep 16, 2013 at 01:39:09AM +0400, Ruslan Kuprieiev wrote:
>> On 09/16/2013 01:12 AM, Cyrill Gorcunov wrote:
>>> On Mon, Sep 16, 2013 at 12:58:49AM +0400, Ruslan Kuprieiev wrote:
>>>> @@ -971,8 +956,11 @@ static inline int fork_with_pid(struct pstree_item *item)
>>>>  	if (ca.clone_flags & CLONE_NEWPID)
>>>>  		item->pid.real = ret;
>>>> -	if (opts.pidfile && root_item == item)
>>>> -		write_pidfile(opts.pidfile, ret);
>>>> +	if (opts.pidfile && root_item == item) {
>>>> +		ret = write_pidfile(opts.pidfile, ret);
>>>> +		if (ret < 0)
>>>> +			pr_perror("Can't write pidfile");
>>>> +	}
>>> I must admit I have no clue what you're guys doing so it
>>> might be by purpose, but don't you need to exit with error
>>> here if write_pidfile failed or it's safe?
>>
>> I've done like it is in previous "if". Monkey see, monkey do=). Also
>> there are "err_unlock" and "err" labels, that are right below.
> 
> Ah, this comes from commit
> 
> commit 827c633b26197fd464551f0d5b22bfcc3653136f
> Author: Pavel Emelyanov <xemul at parallels.com>
> Date:   Tue Nov 27 21:59:58 2012 +0300
> 
>     rst: Write pidfile in separate fn
>     
>     Just for better readability.
>     
>     Signed-off-by: Pavel Emelyanov <xemul at parallels.com>
> 
> and write_pidfile is
> 
> static void write_pidfile(char *pfname, int pid)
> {
> 	int fd;
> 
> 	fd = open(pfname, O_WRONLY | O_TRUNC | O_CREAT, 0600);
> 	if (fd == -1) {
> 		pr_perror("Can't open %s", pfname);
> 		kill(pid, SIGKILL);
> 		return;
> 	}
> 
> 	dprintf(fd, "%d", pid);
> 	close(fd);
> }
> 
> Pavel, why kill() here, don't you need to pass exeution to err_unlock:
> and such?

No, it's root task restore, just killing one is OK.

> .
>