Re: [linux-minidisc] Implementing downloading in QHiMDTransfer

["Kevin Chabowski" <kevin@kch42.de>]

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 <kevin@kch42.de>
> > > | 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

Attachment: 0001-himd_set_track_label-implemented.patch
Description: Binary data

Attachment: binw86OoFQYKl.bin
Description: Binary data