ATF safety

[Jeff Mitchell]

On Friday 18 August 2006 04:45, Martin Aumueller wrote:
> I didn't see anybody try flock or fcntl. Both of these mechanisms were
> created to handle cases as ours. So I think it's worth trying before
> resorting to other mechanisms that involves changes in many places.

Scott Wheeler and I discussed and researched this at length.  Various features 
are not portable, and all of them have serious troubles with NFS and Samba, 
and could silently fail.  Better not to rely on something we know is broken.

> There is still this setUniqueId call that does the actual writing. Which is
> confusing, as this call has the same name as the others, and it behaves
> differently.

Actually it behaves exactly the same, and exactly like the other set* 
functions: it sets the value of m_uniqueId.

> But I see, the situation has improved a bit: the other 
> setUniqueId calls don't call it.

They haven't for weeks.

> But the one w/o parameters is also not in 
> line with the general MetaBundle API pattern: all other setters accept a
> parameter.

It depends what you want to consider the API.  From a strict function 
parameter perspective that's true (although there is the one that accepts a 
parameter, but you should only use it if you know what you're doing).  
However, as it sets the value of m_uniqueId, just as set* sets the value of 
m_*, I think it fits just fine.

The other values of metabundles are designed to be changed arbitrarily.  The 
UID should never be changed, and if it is, it should be immediately written 
to the file; if you don't keep then in sync, then there's no point in having 
them.  And in general use you should never, ever have to change it.

It is true, the function name could be changed slightly, but then you've have 
the non-intuitive case where you call fooUniqueId to set m_uniqueId.  I think 
that's more confusing than the set* way that is used to set all the other 
member variables.

> I still think you can: of course my pseudo code was not complete. Do
> something like this when scanning a file: - read the tags, including a
> unique id if available (but don't yet do anything except for just reading
> the tags) - if no unique id is there, then invent a new one and write it to
> the file
>
> This would mean changing the current code from
>
> MetaBundle bundle( path ); // this is scary, might write to the file if no
> unique id is there!

Only if it's being called by the collectionscanner (or a dcop call), and if 
ATF tag writing is currently turned on.  In which case you *want* it to write 
a UID to the file if it's not there.  That's not scary at all.  If you want I 
can make it explicitly clear with a comment in the code, saying "it gets 
saved in this one," but considering the other two simply either assign a 
string or do a database query, and the other calls scannerSafeSave on a 
TagLib::File, I think it's pretty obvious which does which.

> to
>
> MetaBundle bundle( path ); // would just read tags
> if( bundle.uniqueId.isEmpty() )
> {
>    bundle.setUniqueId( createUniqueId() );
>    bundle.save();
> }

You can't do this.  Various functions populate metabundles in ways that do not 
read the tags of files, but rather fill in values i.e. from the database.  
Worse, they then package a copy of that metabundle up to some list, and then 
re-use it.  The various files may or may not have UIDs, so you can't rely on 
checking for isEmpty, nor can you rely on checking for !isEmpty.  However, 
any of the functions that do this will call setUniqueId() which looks up the 
associated UID from the database.  In fact, this is done for you whenever 
setUrl or setPath is called, since changing the path means that your UID is 
invalidated.

Although you could theoretically pull the call to setUniqueId out of readTags, 
I don't see any reason it doesn't belong there, as it is a tag like any 
other -- and after the first set of a UID, it will be read each scan like any 
other tag.  I could have put this functionality inside readTags itself, but 
that would make readTags bloated, and make it stupidly kludgy to strip or 
change UIDs.

 > - MetaBundle remains a general purpose class with no magic behaviour
> depending on the processs name

It's the KApplication name, not the process name; there's a difference, as 
this is doing it the KDE way.  You *have* to do magic with the name, at least 
at save-time (with the recent changes I could probably take out that check 
from setUniqueId, as the check is made in the new scannerSafeSave, but I'd 
have to think about it), because you must know who is calling it for safety 
reasons.  It's not enough to know whether the scanner is simply running or 
not.  It's still a general purpose class, and if someone wants to reuse it 
somewhere else, it's not hard to strip that check out.

> - only one setUniqueId method in MetaBundle 
> should be sufficient then

You mean two.  Since you don't always (or even usually) read tags, you 
generally must get this value from the database.  Hence setUniqueId().  The 
alternative would be to look at every place setUniqueId() is currently called 
and start duplicating code to do the check there, which is bad, and means it 
will break if someone does a call without the proper check, plus all the 
other downsides of code duplication, etc etc.

> less logic in 
> MetaBundle: no need for processesing 'recreate' or 'strip' options - just
> do a setUnqueId( createUniqueId() ) or setUniqueId( QString::null )
> followed by saving the bundle - the other cases you mentioned would work
> with this scheme easily

You'd then end up with duplicating code again, in readTags and in save.  
Harder to maintain, and makes both functions more bloated.

> In my opinion, removing the special logic for unque ids from MetaBundle and
> doing it where it is necessary

I don't know what you mean.  The logic for creating and removing IDs is in 
MetaBundle, where it belongs, as it is a property of the bundle just like the 
rest of the tags.  The logic that makes ATF work to track files, which is 
database related, is in CollectionDB.  How are those not the correct places?

> would really make it easier to understand 
> what's going on (e.g. I just had a look at collectionscanner.cpp for
> writing this mail and I was thinking, where the hell do the unique ids get
> written, until I found that it happens in readTags!)

Actually, they don't get written in readTags, and that's an important point.  
The reading, writing and stripping code is separate, which allows it to be 
called at various times unassociated with the collectionscanner; there are 
few times readTags is called.  However, keep in mind that this generally 
happens only once per file; all the rest of the times the file may be 
scanned, it's doing a read of the tag and nothing else.  Hence calling the 
function from readTags makes sense.  Alternately it could be called directly 
after readTags, every time readTags is called, but since it's so tightly 
coupled to readTags, I don't see any reason why readTags itself shouldn't 
call it.  Otherwise you have the potential to forget some time.  Plus, you 
avoid code duplication, and have to create a new fileref, yadda yadda.

--Jeff