On Sun, 2012-11-18 at 14:19 +0100, Kevin Chabowski wrote: > Sorry, that I didn't reply earlier. 3rd semester at university is much > more time consuming than I thought... Sorry for the long delay, too. Your patches were not forgotten. > But here are finally fixes for the first 8 (now 5) patches. The first three have been applied to the repository (after deleting one trailing space in the third one). > > | From e096e749587e50da5c72bb0afa7da5bbd0e1c725 Mon Sep 17 00:00:00 2001 > > | From: Kevin Chabowski <kevin@kch42.de> > > | Date: Mon, 22 Oct 2012 21:43:35 +0200 > > | Subject: [PATCH 07/16] himd_set_track_label implemented > > This patch definitely does not pass quality control. While you describe > > being able to change existing labels, and it appears to work fine at the > > beginning, you are leaking HiMD strings. The 14-byte string fragments > > that are unused are organized in a linked freelist. When you change a > > title, you have to add all the string fragments used by the title back > > into the freelist (and change the type nibble to zero) to prevent > > loosing these "string slots" until the next formatting of the disk. > In the new himd_set_track_label patch there is now a function to properly > delete strings, which himd_set_track_label now calls before adding the new > string. 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. For the transactional model, I would suggest you traverse the chain twice - once to check its validity, and a second time to change the flags. In the first round, do not only check that the non-head fragments have the type "continuation", but also that the first fragment is neither unused nor continuation. (Check himd_get_string_raw) You might also want to check the range of the string index (compare there, too). A trap my comments might lead you into is that if you just check the validity in the first round, you could get into an endless loop. There are two ways to fix that: - Do *not* do two iterations, but *do* change the flags from the string type or "continuation" to "unused" (don't change the links, though). If everything is fine, link the last item to the freelist head, and linkt the freelist head to the first item. If something is fishy, undo what you did. (This means you have to store the type field of the first chunk, though) - Count the number of links you followed. If you followed more than 4096 links, you must be in an infinite loop. 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. > label_type is now an enum and is checked using g_return_val_if_fail. Looks fine. > > The himd_modify_track approach is quite dangerous, as it relies on the > > user to pass the right track index to it. > Okay, the patch now introduces himd_start_update_track_info and > himd_commit_update_track_info like you suggested. 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. Are you aware you don't need the parenthesis in &(t->data)? Something I noted, but is not easily fixable (and I explicitly don't ask you to fix, and I forbid you to fix it in the same patch as you do the retitling) is that the current architecture of the code does not easily allow you to make that function efficiently transactional. There is no way to check whether "there will be enough space to store a given string after another string has been deleted". One could implement that by splitting the conversion of the string to the on-disk encoding, the free-space check and the inserting of the data into three pieces, and having the himd_set_string function first convert the new string, then find the length of the old string (in chunks), calculate the number of extra chunks needed and peek for that number of chunks in the free list. Then delete the old string and set the new string as it is done now. > > | From ffd91b653bef249d38d9ecde18a5d993fc8a081d Mon Sep 17 00:00:00 2001 > > | From: Kevin Chabowski <kevin@kch42.de> > > | Date: Mon, 22 Oct 2012 22:15:00 +0200 > > | Subject: [PATCH 08/16] Using himd_set_track_label in himd_writemp3 of > > himdcli > > This patch looks fine, except for error handling (printing a warning) if > > himd_set_track_label fails. > > Okay, prints an error on failure now. This patch is fine now, I didn't apply that because I didn't apply the previous patch. Thank you very much for the time you spent on improving linux-minidisc stuff! Kind regards, Michael Karcher