Call for review && test: linux_kdump-1.6

[Chagin Dmitry]

On Mon, 14 Apr 2008, Kostik Belousov wrote:

> On Mon, Apr 14, 2008 at 08:18:22PM +0400, Chagin Dmitry wrote:
>> On Mon, 14 Apr 2008, Kostik Belousov wrote:
>>
>>> On Mon, Apr 14, 2008 at 03:31:37PM +0300, Kostik Belousov wrote:
>>>>
>>>> Ah, I see. Then, we shall never dump the ERESTART and EJUSTRETURN
>>>> for the emulated ABIs. At least, this is true for Linux, I am not
>>>> so sure about iBCS2 and SVR4.
>>>>
>>
>> I do not absolutely agree with this statement. If the emulated syscalls
>> should return native FreeBSD errno, that why to not write them to a
>> ktrace file without conversion? Current linux_kdump port uses strerror
>> because expects it. At least, it is convenient :)
> Again, could you, please, elaborate ? ABI emulation shall return the
> translated errors. And, the current behaviour is to dump translated
> error codes, so linux_kdump must cope with it already.
>

ABI emulation shall return the translated errors to user-space, but not 
to kernel-space where dump written. But i have understood all, thanks! I 
simply thought, that while linux_kdump unique easier to change rules for 
all following.

>>
>> Your patch is correct for my version of linux_kdump, but does not solve
>> a problem with the current port version.
> I think we could commit the trap.c patch simultaneously with
> the new linux_kdump. Even better, two versions of the linux_kdump
> could coexist in the ports, with the right one being selected based
> on the __FreeBSD_version. But I would leave this to the emulation at .
>

ok

>>
>> If it's possible, explain please, that is not correct in my last patch?
> Your previous patch (cited above) prevents the dumping of the
> EJUSTRETURN for the native FreeBSD syscalls, that is also wrong, IMHO.
>
> Assuming that your last patch is the one that moved the ktrsysret()
> before the switch (error), I see two problems:
> 1. It dumps the error before the ABI compat has translated the error.
>   This is definitely huge deviation with the present behaviour, see
>   above.
> 2. It missed the amd64 trap.c.
> #1 is corrected in my version. #2 is a trivial overlook.
>
>>
>> thnx
>>
>>>> Could you test the patch below, instead ?
>>>
>>>> diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
>>>> index e7de579..55642d1 100644
>>>> --- a/sys/i386/i386/trap.c
>>>> +++ b/sys/i386/i386/trap.c
>>>
>>> The patch is obviously wrong, it just prevents the Linux ENOENT to be
>>> dumped. Please, try this one instead.
> ^^^^^^^^This statement is about _my_ first patch.
>
>>>
>>> diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
>>> index b6a454d..3b1368a 100644
>>> --- a/sys/amd64/amd64/trap.c
>>> +++ b/sys/amd64/amd64/trap.c
>>> @@ -861,9 +861,18 @@ syscall(struct trapframe *frame)
>>> 		frame->tf_rip -= frame->tf_err;
>>> 		frame->tf_r10 = frame->tf_rcx;
>>> 		td->td_pcb->pcb_flags |= PCB_FULLCTX;
>>> -		break;
>>> -
>>> +		/* FALLTHROUGH */
>>> 	case EJUSTRETURN:
>>> +#ifdef KTRACE
>>> +		/*
>>> +		 * The ABIs that use the negative error codes, like
>>> +		 * Linux, would confuse the in-kernel errno values
>>> +		 * with proper userspace errno. Clean these values to
>>> +		 * avoid a confusion in the kdump.
>>> +		 */
>>> +		if (p->p_sysent->sv_errsize)
>>> +			error = 0;
>>> +#endif
>>> 		break;
>>>
>>> 	default:
>>> diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c
>>> index e7de579..6ec04b0 100644
>>> --- a/sys/i386/i386/trap.c
>>> +++ b/sys/i386/i386/trap.c
>>> @@ -1040,9 +1040,18 @@ syscall(struct trapframe *frame)
>>> 		 * int 0x80 is 2 bytes. We saved this in tf_err.
>>> 		 */
>>> 		frame->tf_eip -= frame->tf_err;
>>> -		break;
>>> -
>>> +		/* FALLTHROUGH */
>>> 	case EJUSTRETURN:
>>> +#ifdef KTRACE
>>> +		/*
>>> +		 * The ABIs that use the negative error codes, like
>>> +		 * Linux, would confuse the in-kernel errno values
>>> +		 * with proper userspace errno. Clean these values to
>>> +		 * avoid a confusion in the kdump.
>>> +		 */
>>> +		if (p->p_sysent->sv_errsize)
>>> +			error = 0;
>>> +#endif
>>> 		break;
>>>
>>> 	default:
>>>
>>
>> --
>> Have fun!
>> chd
>

-- 
Have fun!
chd