Re: [PATCH v4] Rewrite iconv option parsing [BZ #19519]

[Carlos O'Donell <carlos at redhat dot com>]

On 2/10/20 2:53 PM, Arjun Shankar wrote:
> Hi Carlos,
> 
> Thanks for the review
> 
> Just about to send a v5, but I wanted to clarify on a couple of your
> review comments before I do so:
> 
>>> +  gconv_parse_code (&pfc);
>>> +  gconv_parse_code (&ptc);
>>> +
>>
>> Suggest:
>>
>> /* We ignore error handlers from the fromcode because that doesn't make
>>    any sense since all errors arise from converting the fromcode to the
>>    tocode.  Therefore, only tocode error handlers are used in the
>>    conversion specifier.  */
>>
>>> +  conv_spec->translit = ptc.translit;
>>> +  conv_spec->ignore = ptc.ignore;
> 
> I was discussing this with Florian and concluded (and I hope he still
> agrees) that in reality, it does make sense to accept the IGNORE error
> handler in fromcode. IGNORE comes into play when the input has invalid
> characters (as per the input character set), and thus it is indeed an option
> that seems to logically apply to the "fromcode". The reason we only parse it
> from the tocode is because that's what the man page states and is now the
> specification. I think we could consider changing this in a forthcoming
> patch. It's still a bit odd and deserves a comment, so I have now
> added a slighty different comment in the code at this point,
> stating that we only parse tocode suffixes because that's what the
> specification states. I also added three test cases to tst-iconv-opt.c to
> verify that options passed in the fromcode are indeed ignored as claimed.

Perfect answer.

My suggestion:

/* We ignore error handlers from the fromcode because that is how the
   current implementation has always handled the error handlers.  The reality
   is that invalid input in the input character set should only be ignored if
   the fromcode specifies IGNORE.  The current implementation ignores invalid
   intput in the input characters set if the tocode contains IGNORE.  We
   preserve this behavior for backwards compatibility.  In the future we may
   split the handling of IGNORE to allow a finer grained specification of
   ignorning invalid input and/or ignoring invalid output.  */

How does that read?

>>> +ICONV='$codir/elf/ld.so --library-path $LIBPATH --inhibit-rpath ${from}.so \
>>> +       $codir/iconv/iconv_prog'
>>
>> Why use '' here? You can just use "" and it should keep parameters separate.
> 
> It is because ${from} hasn't been set yet. It's set at every iteration of
> the loop and thus needs to be expanded at that point. I let this bit of code
> stay as-is in v5 because of the expansion.

OK. I didn't notice that!

>>> +# Requires $twobyte input, $c flag, $from, and $to to be set; sets $ret
>>> +execute_test ()
>>> +{
>>> +  PROG=`eval echo $ICONV`
>>
>> Why do you eval echo through a variable here?
>>
>> I expect this is due to your use of '' above, remove that and you should
>> be able to use $ICONV directly below.
> 
> For the same reason, I've left this bit of code as-is in v5.

OK. Ah! I see you haven't expanded the ${from} yet, and that makes sense.

> Once again, thank you for the review. Hoping for v5 to be more acceptable!
> I've taken care of all other review comments that you had on v4. Patch
> incoming.

Thanks!

-- 
Cheers,
Carlos.