Re: [Libcdio-devel] CD-Text API changes

[Nicolas Boullis]

Hi,

First of all, I apologize for the late response.

On Sat, Feb 25, 2012 at 11:57:03PM +0100, Leon Merten Lohse wrote:
> 
> Now there is (some of it not yet commited):
> cdtext.h:
>   const char *cdtext_genre2str (cdtext_genre_t i); // new
>   const char *cdtext_lang2str (cdtext_lang_t i); // new
>   const char *cdtext_field2str (cdtext_field_t i); // unchanged
>   void cdtext_init (cdtext_t *p_cdtext); // prototype unchanged
>   /* parses binary cd-text information into a cdtext_t struct */
>   int cdtext_data_init(cdtext_t *p_cdtext, uint8_t *wdata, size_t length); // 
> new
>   void cdtext_destroy (cdtext_t *p_cdtext); // unchanged
>   /* new track parameter, changed order */
>   char *cdtext_get (const cdtext_t *p_cdtext, cdtext_field_t key, track_t 
> track);
>   /* new track parameter, changed order */
>   const char *cdtext_get_const (const cdtext_t *p_cdtext, cdtext_field_t key, 
> track_t track);
>   cdtext_lang_t cdtext_get_language (const cdtext_t *p_cdtext); // new, 1)
>   bool cdtext_select_language(cdtext_t *p_cdtext, const char *lang); // new, 
> 2)
>   cdtext_lang_t *cdtext_languages_available (const cdtext_t *p_cdtext); // 
> new, 3)
> disc.h
>   /* returns cd-text binary as it is */
>   uint8_t * cdio_get_cdtext_raw (CdIo_t *p_cdio); // new
>   /* calls cdtext_data_init on the return value of cdio_get_cdtext_raw */
>   cdtext_t *cdio_get_cdtext (CdIo_t *p_cdio); // unchanged

I am a bit curious: why does cdtext_select_language take a "const char *" 
parameter for the language, while cdtext_get_language and 
cdtext_languages_available return cdtext_lang_t value(s)? Wouldn't it be 
more consistent if cdtext_select_language too, a cdtext_lang_t 
parameter?


Now, as for the API and ABI changes...
I have no problem with the API change, as long as this part of the API 
currently is seldom used (if I understand things correctly).
I am more concerned about ABI changes, because the whole library must 
"say" whether it is compatible with previous versions. If it is said to 
be incompatible, then evrything needs to be rebuilt. On the other hand, 
if it is said to be compatible while it is not 100%, then users who use 
the new one with programs built against the old one may experience 
crashes.
So, for me, the question is: is the new ABI compatible with the old one, 
and if it is not, should it be made compatible?

You only specify the API, bu I guess the ABI roughly is a 1-1 mapping of 
the API. Then it means the new ABI is incompatible with the old one. 
Should it be made compatible with the old one?

Thanks to symbol versionning, it should be possible not to break the 
ABI, by shipping old symbols alongside with new ones. For example, we 
may have both cdio_get_cdtext@@CDIO_13 and cdio_get_cdtext@@CDIO_13_1 
symbols, while only the latter is exposed by the API. As far as I know, 
this is roughly how things work with glibc. (It would be unacceptable to 
have the ABI of glibc change often and require a rebuild of roughly 
everything.)

As I understand it, adding the old symbols would require to resurrect 
the old cdtext_t type (with a new name), the functions whose prototype 
has changed (of course) and all those that used the old cdtext_t type 
(since their binary interface changed as well).

Moreover, this would require some more work on symbol versionning, as we 
currently define all symbols with the same version, which we wouln't be 
able to do any more.

Now, is worth the effort? If I were a purist, I'd certainly say it is. 
But being somewhat pragmatic, and despite I hate ABI changes, I think it 
is too much hassle.


Now, a side note: shouldn't CDText functions be kept in a separate 
library, distributed with libcdio, just like libiso9660, libudf and 
libcdio-cdda? This would allow to break the ABI of this new library 
without breaking libcdio's ABI. Any opinion on this?


Cheers,

-- 
Nicolas