check the return when constructing comment entries

[nagisa]

The FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair
function call can fail which will lead to
FLAC__metadata_object_vorbiscomment_append_comment segfaulting or
producing invalid results. I have observed this to happen after 2 days
of normal playback. It is reproducible much more readily if I just skip
a lot.

With this fix in place, even with skipping I haven’t been able to
reproduce the crash again.

[toots]

Thank you for this! I think I'd rather have the encoder raise an exception to let the user know that they are submitting invalid metadata. Otherwise, it might be surprising to think everything went fine while some metadata is missing.

Let me know if you're confortable doing that in the ocaml C code (declare new exception on the ML side, register it for C callback, call it in the C code). Otherwise, I'm happy to do it. The reference for OCaml bindings is here: https://v2.ocaml.org/manual/intfc.html

[nagisa]

I think I'd rather have the encoder raise an exception to let the user know that they are submitting invalid metadata.

Hm, with this being a part of context creation, I wonder how would something like liquidsoap know that it might want to try discarding that metadata?

Let me know if you're confortable doing that in the ocaml C code (declare new exception on the ML side, register it for C callback, call it in the C code). Otherwise, I'm happy to do it. The reference for OCaml bindings is here: https://v2.ocaml.org/manual/intfc.html

Please take over. An unfortunate thing with FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair is that it bundles three distinct failure modes internally, so you may want to figure out how to raise corresponding exceptions for each failure or something.

[toots]

I think I'd rather have the encoder raise an exception to let the user know that they are submitting invalid metadata.

Hm, with this being a part of context creation, I wonder how would something like liquidsoap know that it might want to try discarding that metadata?

I'll think about it from the liquidsoap side. But this made me realize that FLAC__format_vorbiscomment_entry_name_is_legal and FLAC__format_vorbiscomment_entry_value_is_legal are exported too so I'll add them to the binding.

Let me know if you're confortable doing that in the ocaml C code (declare new exception on the ML side, register it for C callback, call it in the C code). Otherwise, I'm happy to do it. The reference for OCaml bindings is here: https://v2.ocaml.org/manual/intfc.html

Please take over. An unfortunate thing with FLAC__metadata_object_vorbiscomment_entry_from_name_value_pair is that it bundles three distinct failure modes internally, so you may want to figure out how to raise corresponding exceptions for each failure or something.

Sounds good. Looks like the errors are either one of the two above or a malloc failure. malloc failures are pretty radical so I think I'll just raise a Invalid_metadata generic error, export the two above functions for checking and expect malloc failure to show up in any other subsequent allocation.