Re: [PATCH 5/5] uprobes/x86: Move default_xol_ops's data into arch_uprobe->def

[Oleg Nesterov <oleg@redhat.com>]

On 04/24, Jim Keniston wrote:
>
> I see a couple of nits in this patch (see below), but the others look
> good.
>
> Patches 1-5 of this set:
> Reviewed-by: Jim Keniston <jkenisto@us.ibm.com>

Thanks!

> >  		struct {
> >  			s32	offs;
> >  			u8	ilen;
> >  			u8	opc1;
> > -		}				branch;
> > +		}			branch;
> > +		struct {
> > +#ifdef CONFIG_X86_64
> > +			long	riprel_target;
> > +#endif
> > +			u16	fixups;
> > +		} 			def;
>
> "def" is kind of ambiguous.

Heh. I am shy to admit that my plan was to name it "default". I changed
its name only after gcc told me I should learn "C".

> How about "dfault" or some such?

Then probably "dflt", this looks more consintent and more IBMish ;)

On a serious note, I agree with any naming... but "dfault" looks like
"d" fault to me. Perhaps something else?

> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> ...
> > @@ -636,12 +635,12 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
> >
> >  	/*
> >  	 * Figure out which fixups arch_uprobe_post_xol() will need to perform,
> > -	 * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups
> > -	 * is either zero or it reflects rip-related fixups.
> > +	 * and annotate def->fixups accordingly. To start with, ->fixups is
> > +	 * either zero or it reflects rip-related fixups.
>
> That sentence stopped being true a couple of patch sets ago.
> handle_riprel_insn() is called later in this function now.

Yes, but the comment mentions arch_uprobe_post_xol? Anyway, I'll update
it to say "default_post_xol_op" instead.

Or I misunderstood you ?

Oleg.