numerical option parsing broken in engine options

numerical option parsing broken in engine options

[Castor Fu]

I was debugging some problems with parsing fio jobs, and found
options.c:fio_get_kb_base is broken for external engines... The
parameter
'data' is either 0, td, or td->eo.  If it's td->eo it's not really
going to work.

The cleaner fix is probably to unwind passing 'data' so far down the
stack, but it
touches a lot of functions, so I thought I'd send this out now.

unsigned int fio_get_kb_base(void *data)
{
        struct thread_options *o = data;
        unsigned int kb_base = 0;

        if (o)
                kb_base = o->kb_base;
        if (!kb_base)
                kb_base = 1024;

        return kb_base;
}


If I replace
   if(o)
by
     if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)

then it behaves as I expect when engine options use the 'k', etc.

    -castor

Re: numerical option parsing broken in engine options

[Jens Axboe]

On 2014-02-24 17:51, Castor Fu wrote:
> I was debugging some problems with parsing fio jobs, and found
> options.c:fio_get_kb_base is broken for external engines... The
> parameter
> 'data' is either 0, td, or td->eo.  If it's td->eo it's not really
> going to work.
>
> The cleaner fix is probably to unwind passing 'data' so far down the
> stack, but it
> touches a lot of functions, so I thought I'd send this out now.
>
> unsigned int fio_get_kb_base(void *data)
> {
>          struct thread_options *o = data;
>          unsigned int kb_base = 0;
>
>          if (o)
>                  kb_base = o->kb_base;
>          if (!kb_base)
>                  kb_base = 1024;
>
>          return kb_base;
> }
>
>
> If I replace
>     if(o)
> by
>       if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)
>
> then it behaves as I expect when engine options use the 'k', etc.

Not sure I follow... Are you calling fio_get_kb_base() from the external 
engine?

-- 
Jens Axboe

Re: numerical option parsing broken in engine options

[Castor Fu]

Indirectly.  If the external engine has an option which is a
FIO_OPT_INT, and I give it a value like  '4k', then we'll eventually
call fio_get_kb_base while pointing to the engine options area instead
of the options / thread_data area.

On Tue, Feb 25, 2014 at 9:07 AM, Jens Axboe <axboe@kernel.dk> wrote:
> On 2014-02-24 17:51, Castor Fu wrote:
>>
>> I was debugging some problems with parsing fio jobs, and found
>> options.c:fio_get_kb_base is broken for external engines... The
>> parameter
>> 'data' is either 0, td, or td->eo.  If it's td->eo it's not really
>> going to work.
>>
>> The cleaner fix is probably to unwind passing 'data' so far down the
>> stack, but it
>> touches a lot of functions, so I thought I'd send this out now.
>>
>> unsigned int fio_get_kb_base(void *data)
>> {
>>          struct thread_options *o = data;
>>          unsigned int kb_base = 0;
>>
>>          if (o)
>>                  kb_base = o->kb_base;
>>          if (!kb_base)
>>                  kb_base = 1024;
>>
>>          return kb_base;
>> }
>>
>>
>> If I replace
>>     if(o)
>> by
>>       if (o && (data - (void *) threads) % sizeof(threads[0]) == 0)
>>
>> then it behaves as I expect when engine options use the 'k', etc.
>
>
> Not sure I follow... Are you calling fio_get_kb_base() from the external
> engine?
>
> --
> Jens Axboe
>

Re: numerical option parsing broken in engine options

[Jens Axboe]

On 2014-02-25 09:34, Castor Fu wrote:
> Indirectly.  If the external engine has an option which is a
> FIO_OPT_INT, and I give it a value like  '4k', then we'll eventually
> call fio_get_kb_base while pointing to the engine options area instead
> of the options / thread_data area.

Ah I see now, yes that is definitely an issue... Let me think about this 
a bit. td->eo as data only works for the engine itself, it's not pretty 
that the parser ends up thinking it's td.

-- 
Jens Axboe

Re: numerical option parsing broken in engine options

[Jens Axboe]

On 2014-02-25 10:52, Jens Axboe wrote:
> On 2014-02-25 09:34, Castor Fu wrote:
>> Indirectly.  If the external engine has an option which is a
>> FIO_OPT_INT, and I give it a value like  '4k', then we'll eventually
>> call fio_get_kb_base while pointing to the engine options area instead
>> of the options / thread_data area.
>
> Ah I see now, yes that is definitely an issue... Let me think about this
> a bit. td->eo as data only works for the engine itself, it's not pretty
> that the parser ends up thinking it's td.

So the issue is really that a non static function makes assumptions 
about what 'data' is at this point. That's just not going to work. 
Internal use cases are fine, since they know what *data is, but it wont 
work for fio_get_kb_base().

So we definitely need some way of knowing if 'data' is our regular 
options or not...

-- 
Jens Axboe

Re: numerical option parsing broken in engine options

[Jens Axboe]

On 2014-02-25 11:18, Jens Axboe wrote:
> On 2014-02-25 10:52, Jens Axboe wrote:
>> On 2014-02-25 09:34, Castor Fu wrote:
>>> Indirectly.  If the external engine has an option which is a
>>> FIO_OPT_INT, and I give it a value like  '4k', then we'll eventually
>>> call fio_get_kb_base while pointing to the engine options area instead
>>> of the options / thread_data area.
>>
>> Ah I see now, yes that is definitely an issue... Let me think about this
>> a bit. td->eo as data only works for the engine itself, it's not pretty
>> that the parser ends up thinking it's td.
>
> So the issue is really that a non static function makes assumptions
> about what 'data' is at this point. That's just not going to work.
> Internal use cases are fine, since they know what *data is, but it wont
> work for fio_get_kb_base().
>
> So we definitely need some way of knowing if 'data' is our regular
> options or not...

This is the best short term fix I could come up with...

http://git.kernel.dk/?p=fio.git;a=commit;h=cb1402d674fb7694d1b09d7ef4aeb3d4506f23f0

It'd be nice if kb_base always worked as expected, however. So I'll keep 
mulling over this and see if we can't come up with something better. We 
might just have to have two sets of private, or a private container so 
we can always pass in the right thing.

-- 
Jens Axboe