Fix seg fault when using iconv_open()

[postscript-dev]

• Fix checking for failure with icon_open(). From the iconv_open(3) manpage:

RETURN VALUE On success, iconv_open() returns a freshly allocated conversion descriptor. On failure, it returns (iconv_t) -1 and sets errno to indicate the error. 

• Add new exception when failing to change the text encoding of an Exif comment

Bug is on the main branch and does not affect exiv2 0.27.5.

[codecov]

Codecov Report

Merging #2403 (7d69936) into main (dc5dc0d) will increase coverage by 0.18%.
The diff coverage is 75.00%.

@@ Coverage Diff @@ ## main #2403 +/- ## ========================================== + Coverage 64.28% 64.46% +0.18%  ========================================== Files 119 119 Lines 21135 21139 +4 Branches 10430 10435 +5 ========================================== + Hits 13586 13627 +41  + Misses 5400 5353 -47  - Partials 2149 2159 +10 

Impacted Files	Coverage Δ
include/exiv2/error.hpp	50.00% <ø> (ø)
src/error.cpp	83.60% <ø> (ø)
src/convert.cpp	51.55% <50.00%> (+0.15%)	⬆️
src/value.cpp	72.59% <100.00%> (+0.40%)	⬆️
app/exiv2.cpp	63.43% <0.00%> (+0.89%)	⬆️
app/actions.cpp	67.18% <0.00%> (+2.20%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[piponazo]

Changes look good to me. Thanks for the additional error checking!

[piponazo]

[suggestion] could you define and assign the cd in the same line? Furthermore, is it really needed the cast to iconv_t if we already know that the variable cd is of that type?

[neheb]

musl libc (and most others) have it as
typedef void *iconv_t;

uclibc-ng has it as
typedef long iconv_t;

cast is needed if it's a pointer I think.

This does trigger a clang-tidy check that it's a C cast.

I wonder if there's a C++ way to do this...

[postscript-dev]

@neheb

musl libc (and most others) have it as
typedef void *iconv_t;

uclibc-ng has it as
typedef long iconv_t;

Thanks for letting me know, I wasn't aware of the different definitions. As for a C++ solution, would using std::is_pointer_v help?

[neheb]

No i mean you can't use static_cast or reinterpret_cast as it would fail with void* and long respectively.

if std::is_pointer_v<cd> ? reinterpret_cast<iconv_t>(-1) : static_cast<iconv_t>(-1)) 

is quite verbose.

if (iconv_t(-1)) 

is an alternative but newer clang-tidy warns on that as well.

[postscript-dev]

@neheb

musl libc (and most others) have it as
typedef void *iconv_t;

uclibc-ng has it as
typedef long iconv_t;

cast is needed if it's a pointer I think.

This does trigger a clang-tidy check that it's a C cast.
I wonder if there's a C++ way to do this...

While looking at one of your current PRs, I was reminded of this discussion. As we are about to release a new version based on the main branch, are changes needed?

[neheb]

Not really. The clang-tidy check is just stylistic.

[postscript-dev]

@neheb

Not really. The clang-tidy check is just stylistic.

Thanks for checking, I was wondering about this but was pushed for time.