Re: [linux-minidisc] Implementing downloading in QHiMDTransfer

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

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