Mailinglist Archive: zypp-devel (66 mails)

[Michael Andres <ma@xxxxxxx>]

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@xxxxxxxxxx
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@xxxxxxxxxxxx
For additional commands, e-mail: zypp-devel+help@xxxxxxxxxxxx