On Wednesday 25 February 2009 09:58:36 Duncan Mac-Vicar P. wrote:
Hi guys, I need some feedback.
The ultimate goal of this is to be able to have more fine grained control over the protocols, to make easily to add options that alter the transfer behavior. A concrete example is range support. So one can ask for a piece of the file. This could be used to implement changelog retrieval on real time, as we know from the metadata the rpm header range, so we can retrieve the header without the payload. Currently most functions of MediaManager take only the path. Some others also take a bool (ugh!) to change some behavior.
As inspiration, I checked various similar APIs. The design I like most is KIO (KDE's vfs). KIO provides two interfaces, one sync, which has methods like KIO::NetAccess::download(url), and a async one, using jobs. Jobs carry the operation, options, and the important part: metadata. Also the jobs signals back errors, success and data events.
The metadata in KIO is a simple map string->string, which is interpreted by each protocol. Some string are "predefined": (see kdelibs/kio/DESIGN.metadata [1]) while others are ignored by some protocols, or turned into headers by others (http for example).
nice.
As discussed with Michael, we did not want to use OnMediaLocation here, because it has too much information which the MediaManager does not need (like checksums), and it will be APIwise confusing if all those options are ignored at that layer.
OnMediaLocation belongs to repositories and their metadata. Ideally the media backend is mostly independent from zypp (except for somme basic stuff like url, pathname, ManagedFiles, loging ).
I tried various refactorings, and I think at least 3 or 4 class names. The first refactoring consisted in emulating something like OnMediaLocation but lighter, however, changing the Pathname to a new class: - makes the interface inconsistent as lot of methods like "isAttached" or "localPath" still take paths, and they don't need options - If we decide to keep the old methods, the API grows exponentially
Let's take a whiteboard and carefully think about this. The current API is still based on the old y2-packagemanger. Y2pm used methods returning errorcodes and no exceptions. And it's also been a verry different workflow. y2pm required much more control and longterm access to the media. Many of these constraints are gone. Why not leaving the Media* as it is and start building a 'modern' interface in parallel. And switch once the new interface is powerfull enough. Esp. with better exeption handling and reporting.
So, at the end, I ended with adding a MediaOptions class, which can carry the metadata, and anything added later. This options parameter is passed in addition to the Pathname, and it defaults to a empty options, so the API stays the same (source compatible).
I somewhat dislike the idea of separating path and other pathspecific request options. (sounds more like a MediaRequest to me, not Options). Basically you want to describe your request REQUEST: Type=FILE source: Path=/content.gz Size=3544 alt source: Path=/content Size=123546 ( or Range=13..25 ) REQUEST: Type=DIR source: Path=/suse/setup/slide mode=secure recursive=true glob=*gz Instead of using Pathname mediaGet( const Pathname &, const MediaOptions & = MediaOptions ); Pathname mediaGet( const ThisType &, const MediaOptions & = MediaOptions ); Pathname mediaGet( const ThatType &, const MediaOptions & = MediaOptions ); you should do Pathname mediaGet( const MediaRequest & ); and spend a type conversion ctor in MediaRequest: MediaRequest( const Pathname & p ) //=> REQUEST: Type=FILE, source: Path=p So you can still call mediaGet( "/dir/somefile" ); You can then drive it furthrt and think about about doing it this way: Allow MediaRequest construction from every type which has a makeMediaRequest conversion function defined. So you define the ctor together with the argument class. class MediaRequest; template<class _Tp> MediaRequest makeMediaRequest( const _Tp & t ); // media request builder class MediaRequest { public: template<class _Tp> MediaRequest( const _Tp & t ) { *this = makeMediaRequest( t ); } // plus default and copyctor }; // Maybe defined for Pathname as default template<> MediaRequest makeMediaRequest<Pathname>( const Pathname & p ) { MediaRequest ret; // fill request from Pathname return ret; } in OnMediaLocation or even some separate header file: #include OnMediaLocation #include MediaRequest template<> MediaRequest makeMediaRequest<OnMediaLocation>( const OnMediaLocation & ml ) { MediaRequest ret; // fill request suitable for OnMediaLocation return ret; }
MediaSetAccess needs to expose these options. However, as MediaSetAccess main responsability is workflow handling, it already got a ProvideFileOptions on their methods which carry workflow specific options (like non-interactive flag). Therefore while thinking where to have those options, I kind of realized these options fit in OnMediaLocation, so I added OnMediaLocation::mediaOptions(). That would
No, IMO they don't fit in OnMediaLocation. Don't create monsters. OnMediaLocation is representation of metadata. It has nothing to do with MediaOptions. The MediaOptions depend on the context that uses OnMediaLocation, not on OnMediaLocations content. Don't add a member for this. Don't mix desired syntax and semantics. The above MediaRequest could be the builder. You construct the builder object from e.g. Pathname or OnMediaLocation or whatever type may describe the location of a file on a media. Then you set the additional options in the builder, which depend on the context. OnMediaLocation loc; MediaRequest( loc ).setNonInteractive().setOptional() Also there may be different ways to build a MediaRequest from OnMediaLocation. You don't want to add a member method for this and a method for that. MediaRequest makeMediaRequestAlternate( const OnMediaLocation & ml ) {...} MediaRequest makeMediaRequestOnSunday( const OnMediaLocation & ml ) {...} ... OnMediaLocation loc; mediaGet( loc ); // aka mediaGet( makeMediaRequest( loc ) ); mediaGet( makeMediaRequestAlternate( loc ) ); mediaGet( makeMediaRequestOnSunday( loc ) ); mediaGet( makeMediaRequestAlternate( loc ).setNonInteractive() ); or Pathname mediaGetNonInteractive( const MediaRequest & r ) { return mediaGet( r.setNonInteractive() ); } mediaGetNonInteractive( loc ); and so on...
keep the APIs very similar, but adding some more power.
So in summary, my current proposal is:
- MediaManager:: methods and down to MediaHandler:: get an additional parameter media::MediaOptions, which carry the metadata. - OnMediaLocation carries a media::MediaOptions too. (yes, I have a patch :-) ) One thing I don't have clear is how to expose the mediaOptions() from OnMediaLocation. If I just create setter and getter, then it would be exposed by copy or const ref. Would it make sense for mediaOptions() to return a non-const ref so one could set the options directly ( loc.mediaOptions().addMetadata(foo,bar) ), or would it be better to wrap the MediaOptions methods in OnMediaLocation and exposing the object as const ref?
Comments welcome. Is ok to continue in this direction?
Roughly. -- cu, Michael Andres +------------------------------------------------------------------+ Key fingerprint = 2DFA 5D73 18B1 E7EF A862 27AC 3FB8 9E3A 27C6 B0E4 +------------------------------------------------------------------+ Michael Andres YaST Development ma@novell.com SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg) Maxfeldstrasse 5, D-90409 Nuernberg, Germany, ++49 (0)911 - 740 53-0 +------------------------------------------------------------------+ -- To unsubscribe, e-mail: zypp-devel+unsubscribe@opensuse.org For additional commands, e-mail: zypp-devel+help@opensuse.org