Am 16.12.2012, 15:40 Uhr, schrieb Michael Karcher <Michael.Karcher@fu-berlin.de>:
> | From e096e749587e50da5c72bb0afa7da5bbd0e1c725 Mon Sep 17 00:00:00 2001> | From: Kevin Chabowski <firstname.lastname@example.org> > | Date: Mon, 22 Oct 2012 21:43:35 +0200 > | Subject: [PATCH 07/16] himd_set_track_label implemented
This looks fine now, but some comments on the himd_delete_string function: It would be nice if it implemeted a transactional model, if easily possible (i.e. it should change nothing if it returns -1), and it should check for error conditions wherevere reasonable to prevent messing up a partly invalid disc.
Okay, I implemented the first version: It changes the types of the chunks in the string to unused and if something went wrong, it will restore the chain, before returning an error. A strange behavior I observed: On one of my discs (recorded with SonicStage using an RH10) the renaming process fails completely. Although the old string seemed to be deleted correctly (the delete function didn't return -1) and the new string was also written, the old string was still there in the next track-listing. To make things even more weird, the new string was much shorter than the old one, so it would not occupy the same amount of string chunks. Still, the old string was there with intact linking. It worked just fine on another disc. I double-checked that the write protection of the HiMD was deactivated. I ran the program step-by-step in a debugger, still I do not have a clue, what goes wrong here. (btw. is there a simpler way to enable debug infos than manually manipulating the generated makefiles?) Is this a known issue?
Besides, as a style issue, please put a space around the "*" character when declaring pointer variables and arguments as it is done in the other parts of that file, too.
Okay, changed that.
There is one detail I dislike, but on a second look, it might be fine: himd_commit_track_info takes an himderrinfo parameter, but given valid parameters, it can not fail. So it could be a void function, too. (Use g_return_if_fail instead of g_return_val_if_fail). It might be fine if you intend to have it easier adapted in case settrack might return an error.
Yes, it will be easier, if settrack changes. Also I personally think it is not good, if the function would not indicate an error if it had an error (a simple return from a void function). Since most (all?) functions that can return an error code also have a himderrinfo parameter, I included that one, too. Other functions also do this (e.g. himd_add_track_info). So at the moment I left it that way. Of course I can change this, if it is bad style. (But I added a `(void)status;`, so the compiler does not complain about an unused parameter.)
Are you aware you don't need the parenthesis in &(t->data)?
I personally think, it makes it more readable to wrap it in parenthesis. I now left it this way, but again, I will change this, if it bugs you. Kevin
Description: Binary data
Description: Binary data