util: disable log service in cr_system_userns if error fd was -1

Submitted by Stanislav Kinsburskiy on April 22, 2016, 3:23 p.m.

Details

Message ID 20160422152304.144868.68980.stgit@skinsbursky-vz7.qa.sw.ru
State Rejected
Series "util: disable log service in cr_system_userns if error fd was -1"
Headers show

Patch hide | download patch | download mbox

diff --git a/criu/util.c b/criu/util.c
index dae6031..12ce8b4 100644
--- a/criu/util.c
+++ b/criu/util.c
@@ -641,9 +641,6 @@  int cr_system_userns(int in, int out, int err, char *cmd,
 		    move_img_fd(&err, STDIN_FILENO))
 			goto out_chld;
 
-		if (stop_log_fd)
-			log_fini();
-
 		if (in < 0) {
 			close(STDIN_FILENO);
 		} else {
@@ -660,6 +657,9 @@  int cr_system_userns(int in, int out, int err, char *cmd,
 		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
 			goto out_chld;
 
+		if (stop_log_fd)
+			clear_bit(LOG_FD_OFF, sfd_map);
+
 		execvp(cmd, argv);
 
 		pr_perror("exec failed");

Comments

Pavel Emelianov April 22, 2016, 3:58 p.m.
On 04/22/2016 06:23 PM, Stanislav Kinsburskiy wrote:
> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
> After move, log_fd is closed, and thus log service is broken and any error
> prints will disappear.
> This patch disables log service, thus redirecting error output to STDERR
> (which is log fd in this case).
> 
> Calling of log fini is:
> 1) to early: reopen_fd_as_nocheck will fail
> 2) to much: close_safe will print error message, because log fd was already
> closed
> 
> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
> ---
>  criu/util.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/criu/util.c b/criu/util.c
> index dae6031..12ce8b4 100644
> --- a/criu/util.c
> +++ b/criu/util.c
> @@ -641,9 +641,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>  		    move_img_fd(&err, STDIN_FILENO))
>  			goto out_chld;
>  
> -		if (stop_log_fd)
> -			log_fini();
> -
>  		if (in < 0) {
>  			close(STDIN_FILENO);
>  		} else {
> @@ -660,6 +657,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>  		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>  			goto out_chld;
>  
> +		if (stop_log_fd)
> +			clear_bit(LOG_FD_OFF, sfd_map);

Messing directly with sfd_map here is bad idea. Add call __log_fini()
that finis the log with dead file descriptor.

> +
>  		execvp(cmd, argv);
>  
>  		pr_perror("exec failed");
> 
> _______________________________________________
> CRIU mailing list
> CRIU@openvz.org
> https://lists.openvz.org/mailman/listinfo/criu
> .
>
Stanislav Kinsburskiy April 22, 2016, 4:02 p.m.
22.04.2016 17:58, Pavel Emelyanov пишет:
> On 04/22/2016 06:23 PM, Stanislav Kinsburskiy wrote:
>> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
>> After move, log_fd is closed, and thus log service is broken and any error
>> prints will disappear.
>> This patch disables log service, thus redirecting error output to STDERR
>> (which is log fd in this case).
>>
>> Calling of log fini is:
>> 1) to early: reopen_fd_as_nocheck will fail
>> 2) to much: close_safe will print error message, because log fd was already
>> closed
>>
>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>> ---
>>   criu/util.c |    6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/criu/util.c b/criu/util.c
>> index dae6031..12ce8b4 100644
>> --- a/criu/util.c
>> +++ b/criu/util.c
>> @@ -641,9 +641,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>   		    move_img_fd(&err, STDIN_FILENO))
>>   			goto out_chld;
>>   
>> -		if (stop_log_fd)
>> -			log_fini();
>> -
>>   		if (in < 0) {
>>   			close(STDIN_FILENO);
>>   		} else {
>> @@ -660,6 +657,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>   		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>>   			goto out_chld;
>>   
>> +		if (stop_log_fd)
>> +			clear_bit(LOG_FD_OFF, sfd_map);
> Messing directly with sfd_map here is bad idea. Add call __log_fini()
> that finis the log with dead file descriptor.

The problem here is not in log_fini(), but in close_service_fd(). Even 
in close_safe(), to be precise.
What if I add check for valid fd to close_safe()?

>> +
>>   		execvp(cmd, argv);
>>   
>>   		pr_perror("exec failed");
>>
>> _______________________________________________
>> CRIU mailing list
>> CRIU@openvz.org
>> https://lists.openvz.org/mailman/listinfo/criu
>> .
>>
Pavel Emelianov April 22, 2016, 4:16 p.m.
On 04/22/2016 07:02 PM, Stanislav Kinsburskiy wrote:
> 
> 
> 22.04.2016 17:58, Pavel Emelyanov пишет:
>> On 04/22/2016 06:23 PM, Stanislav Kinsburskiy wrote:
>>> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
>>> After move, log_fd is closed, and thus log service is broken and any error
>>> prints will disappear.
>>> This patch disables log service, thus redirecting error output to STDERR
>>> (which is log fd in this case).
>>>
>>> Calling of log fini is:
>>> 1) to early: reopen_fd_as_nocheck will fail
>>> 2) to much: close_safe will print error message, because log fd was already
>>> closed
>>>
>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>> ---
>>>   criu/util.c |    6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/criu/util.c b/criu/util.c
>>> index dae6031..12ce8b4 100644
>>> --- a/criu/util.c
>>> +++ b/criu/util.c
>>> @@ -641,9 +641,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>>   		    move_img_fd(&err, STDIN_FILENO))
>>>   			goto out_chld;
>>>   
>>> -		if (stop_log_fd)
>>> -			log_fini();
>>> -
>>>   		if (in < 0) {
>>>   			close(STDIN_FILENO);
>>>   		} else {
>>> @@ -660,6 +657,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>>   		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>>>   			goto out_chld;
>>>   
>>> +		if (stop_log_fd)
>>> +			clear_bit(LOG_FD_OFF, sfd_map);
>> Messing directly with sfd_map here is bad idea. Add call __log_fini()
>> that finis the log with dead file descriptor.
> 
> The problem here is not in log_fini(), but in close_service_fd(). Even 
> in close_safe(), to be precise.
> What if I add check for valid fd to close_safe()?

No. You just need to unmark service fd that has the fd closed, right?
Make special funciton for this and call it here. It's just to keep
the API clear.

>>> +
>>>   		execvp(cmd, argv);
>>>   
>>>   		pr_perror("exec failed");
>>>
>>> _______________________________________________
>>> CRIU mailing list
>>> CRIU@openvz.org
>>> https://lists.openvz.org/mailman/listinfo/criu
>>> .
>>>
> 
> .
>
Stanislav Kinsburskiy April 22, 2016, 4:22 p.m.
22.04.2016 18:16, Pavel Emelyanov пишет:
> On 04/22/2016 07:02 PM, Stanislav Kinsburskiy wrote:
>>
>> 22.04.2016 17:58, Pavel Emelyanov пишет:
>>> On 04/22/2016 06:23 PM, Stanislav Kinsburskiy wrote:
>>>> In case of error fd was negative, log_fd is _moved_ as STDERR for the child.
>>>> After move, log_fd is closed, and thus log service is broken and any error
>>>> prints will disappear.
>>>> This patch disables log service, thus redirecting error output to STDERR
>>>> (which is log fd in this case).
>>>>
>>>> Calling of log fini is:
>>>> 1) to early: reopen_fd_as_nocheck will fail
>>>> 2) to much: close_safe will print error message, because log fd was already
>>>> closed
>>>>
>>>> Signed-off-by: Stanislav Kinsburskiy <skinsbursky@virtuozzo.com>
>>>> ---
>>>>    criu/util.c |    6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/criu/util.c b/criu/util.c
>>>> index dae6031..12ce8b4 100644
>>>> --- a/criu/util.c
>>>> +++ b/criu/util.c
>>>> @@ -641,9 +641,6 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>>>    		    move_img_fd(&err, STDIN_FILENO))
>>>>    			goto out_chld;
>>>>    
>>>> -		if (stop_log_fd)
>>>> -			log_fini();
>>>> -
>>>>    		if (in < 0) {
>>>>    			close(STDIN_FILENO);
>>>>    		} else {
>>>> @@ -660,6 +657,9 @@ int cr_system_userns(int in, int out, int err, char *cmd,
>>>>    		if (reopen_fd_as_nocheck(STDERR_FILENO, err))
>>>>    			goto out_chld;
>>>>    
>>>> +		if (stop_log_fd)
>>>> +			clear_bit(LOG_FD_OFF, sfd_map);
>>> Messing directly with sfd_map here is bad idea. Add call __log_fini()
>>> that finis the log with dead file descriptor.
>> The problem here is not in log_fini(), but in close_service_fd(). Even
>> in close_safe(), to be precise.
>> What if I add check for valid fd to close_safe()?
> No. You just need to unmark service fd that has the fd closed, right?
> Make special funciton for this and call it here. It's just to keep
> the API clear.

This special function will look like this:

void __log_fini(void)
{
         int fd;

         fd = get_service_fd(LOG_FD_OFF);
         if (fd < 0)
                 return 0;

         clear_bit(type, sfd_map);
         return 0;
}

Is it ok?