Hi, few comments to readability of code ( disclaimer: all code is not tested, so it can contain errors) (disclaimer2: it is my personal opinions and is based on my non-storage knowledge) :
On Tue, 20 Dec 2011 13:10:31 -0000 [email protected] wrote: > Author: fehr > Date: Tue Dec 20 14:10:30 2011 > New Revision: 67038 > > URL: http://svn.opensuse.org/viewcvs/yast?rev=67038&view=rev > Log: > fix proposal to reuse also larger root filesystems (bnc#727362) > > Modified: > trunk/storage/package/yast2-storage.changes > trunk/storage/storage/src/modules/StorageProposal.ycp > > Modified: trunk/storage/package/yast2-storage.changes > URL: > http://svn.opensuse.org/viewcvs/yast/trunk/storage/package/yast2-storage.changes?rev=67038&r1=67037&r2=67038&view=diff > ============================================================================== > --- trunk/storage/package/yast2-storage.changes (original) +++ > trunk/storage/package/yast2-storage.changes Tue Dec 20 14:10:30 2011 > @@ -1,4 +1,9 @@ > ------------------------------------------------------------------- > +Tue Dec 20 14:08:55 CET 2011 - [email protected] + > +- fix proposal to reuse also larger root filesystems (bnc#727362) > + > +------------------------------------------------------------------- > Tue Dec 06 14:08:41 CET 2011 - [email protected] > > - add nofail for volumes using iSCSI disks (bnc#734786) > > Modified: trunk/storage/storage/src/modules/StorageProposal.ycp > URL: > http://svn.opensuse.org/viewcvs/yast/trunk/storage/storage/src/modules/StorageProposal.ycp?rev=67038&r1=67037&r2=67038&view=diff > ============================================================================== > --- trunk/storage/storage/src/modules/StorageProposal.ycp (original) > +++ trunk/storage/storage/src/modules/StorageProposal.ycp Tue Dec 20 > 14:10:30 2011 @@ -413,6 +413,14 @@ map<string, boolean> swapable = > $[]; map<string, boolean> ishome = $[]; > > +void fill_ishome( list<map> pl ) > + { > + foreach( map p, pl, > + ``{ I think that `` is no longer needed in ycp > + if( !haskey( ishome, p["device"]:"" )) > + ishome[p["device"]:""] = Storage::DetectHomeFs(p); > + }); > + } > > global void flex_init_swapable(map<string, map> tg) > { > @@ -2614,13 +2622,13 @@ > return( ret ); > } > > -list<map> can_mp_reuse( string mp, integer min, integer max, > - list<map> partitions ) > +list<map> can_home_reuse( integer min, integer max, > + list<map> partitions ) > { > list<map> ret = []; > if( max>0 ) > max = max + max/10; Not related to this commit, but WTF? any comment would be nice if you do "magic" > - y2milestone( "can_mp_reuse mp %1 min %2 max %3", mp, min, max ); > + y2milestone( "can_home_reuse min %1 max %2", min, max ); > list<map> pl = []; > pl = filter( map p, partitions, > ``(!p["delete"]:false && > @@ -2631,23 +2639,79 @@ > p["size_k"]:0/1024 >= min && > (max==0 || p["size_k"]:0/1024 <= max) && > Storage::CanEdit( p, false ))); > - y2milestone( "can_mp_reuse normal %1", pl ); > + y2milestone( "can_home_reuse normal %1", pl ); > if( size(pl)>0 ) > { > pl = sort( map a, map b, pl, > ``(a["size_k"]:0>b["size_k"]:0)); > - y2milestone( "can_mp_reuse sorted %1", pl ); > + fill_ishome( pl ); > + pl = (list<map>) union( > + filter( map p, pl, ``(ishome[p["device"]:""]:false) ), > + filter( map p, pl, ``(!ishome[p["device"]:""]:false) )); What is purpose of reusing variable pl ( horrible name )? it is not easy to figure out what happen there ( well you reorder list to have part with home on beginning ). I think that sort function fit better here. > + y2milestone( "can_home_reuse sorted %1", pl ); > ret = maplist( map p, partitions, > ``{ > if( !p["delete"]:false && > p["device"]:""==pl[0,"device"]:"" ) > { > - p = Storage::SetVolOptions( p, mp, > PropDefaultFs(), > + p = Storage::SetVolOptions( p, "/home", > PropDefaultFs(), "", "", "" ); > } > return( p ); > }); > } > - y2milestone( "can_mp_reuse ret %1", ret ); > + y2milestone( "can_home_reuse ret %1", ret ); > + return( ret ); > + } > + > +list<map> can_root_reuse( integer min, integer max, > + list<map> partitions ) it looks like these two functions have a lot of similar code, so maybe it can share it and refactor it out to separate methods. > + { > + list<map> ret = []; > + if( max>0 ) > + max = max + max/10; > + y2milestone( "can_root_reuse min %1 max %2", min, max ); > + list<map> pl = []; > + pl = filter( map p, partitions, > + ``(!p["delete"]:false && > + p["fsid"]:Partitions::fsid_native == > + Partitions::fsid_native && > + !Storage::IsUsedBy(p) && > + size(p["mount"]:"")==0 && > + p["size_k"]:0/1024 >= min && > + Storage::CanEdit( p, false ))); > + y2milestone( "can_root_reuse normal %1", pl ); > + if( size(pl)>0 ) These note is right also for above method: think about someone who reads a code. If he want to know what happen when size is zero, then he must look over whole method to see that nothing happen so from my POV it is better to write if( size(pl)==0 ) { return ret; } and then continue without indentation. > + { > + fill_ishome( pl ); from this line to ... > + list<map> p1 = sort( map a, map b, > + filter( map p, pl, > ``(ishome[p["device"]:""]:false)), > + ``(a["size_k"]:0<b["size_k"]:0)); > + y2milestone( "can_root_reuse p1 %1", p1 ); > + list<map> p2 = sort( map a, map b, > + filter( map p, pl, > + > ``(!ishome[p["device"]:""]:false&&p["size_k"]:0/1024>max)), > + ``(a["size_k"]:0<b["size_k"]:0)); > + y2milestone( "can_root_reuse p2 %1", p2 ); > + list<map> p3 = sort( map a, map b, > + filter( map p, pl, > + > ``(!ishome[p["device"]:""]:false&&p["size_k"]:0/1024<=max)), > + ``(a["size_k"]:0>b["size_k"]:0)); > + y2milestone( "can_root_reuse p3 %1", p3 ); > + pl = (list<map>) union( p2, p1 ); > + pl = (list<map>) union( p3, pl ); this line all you do is special sort, nothing more in a lot of complicated lines. I think you can rewrite it this way ( see also that I in comment highlight is something is quite unexpected). Maybe there should be also reason why it is sorted descending in case of size bigger then max : //sort device to have old non-home partition first sorted by size ascending, then old home smaller then max sorted by size ascending and then old home bigger then max sorted *descending* pl = sort( map a, map b, pl, { boolean is_home_a = ishome[a["device]:""]:false; //yes shortcut to ycp map buildins boolean is_home_b = ishome[b["device]:""]:false; if (is_home_a != is_home_b) { //if b is home, then a not and a should be before b and if b is not home, then a is and b should be before a return is_home_a; } boolean a_bigger_max = a["size_k"]:0/1024<=max; //FIXME 1024 should be constant boolean b_bigger_max = b["size_k"]:0/1024<=max; if (a_bigger_max && b_bigger_max) { return a["size_k"]:0>b["size_k"]:0; } else { return a["size_k"]:0<b["size_k"]:0 } } It is shorted, easier to read and reader is aware all of strange behavior directly. > + y2milestone( "can_root_reuse sorted %1", pl ); > + ret = maplist( map p, partitions, > + ``{ > + if( !p["delete"]:false && This is really hard to parse to anyone who is not good in ycp. Does it mean that if "delete" is not set it is true or false? ( of course maybe it is already set and this is example how horrible is this "feature" ) > + p["device"]:""==pl[0,"device"]:"" ) > + { > + p = Storage::SetVolOptions( p, "/", > PropDefaultFs(), > + "", "", "" ); There should be comment what it actually do. From first view it looks magic...and also second look doesn't help me. > + } > + return( p ); > + }); > + } > + y2milestone( "can_root_reuse ret %1", ret ); > return( ret ); > } > > @@ -2916,11 +2980,7 @@ > Storage::CanDelete( p, disk, false > ))); if( size(pl)>0 ) > { > - foreach( map p, pl, > - ``{ > - if( !haskey( ishome, p["device"]:"" )) > - ishome[p["device"]:""] = Storage::DetectHomeFs(p); > - }); > + fill_ishome( pl ); Good, nice to refactor out code to separate method. > pl = sort( map a, map b, pl, > ``(a["size_k"]:0>b["size_k"]:0)); > list<map> l1 = filter( map p, pl, ``(!Storage::IsUsedBy(p)) > ); @@ -3197,28 +3257,24 @@ > if( mode == `reuse ) > { > list<map> parts = disk["partitions"]:[]; > - if( GetProposalHome() ) > + list<map> tmp = []; variable with so big scope ( more then few lines (good limit is e.g. 5 lines) should have better name explaning what it is. For this specific code more reccomend create two separated variables for each case and scope will be limited and also number of lines is limited. > + if( GetProposalHome() && > avail_size>opts["home_limit"]:0 ) { > - if( avail_size > opts["home_limit"]:0 ) > - parts = can_mp_reuse( "/home", 4*1024, > 0, parts ); > - else > - parts = can_mp_reuse( "/", > opts["root_base"]:0, 0, > - parts ); > - } > - if( size(parts)>0 && avail_size > > opts["home_limit"]:0 ) > - { > - integer mx = 0; > - if( GetProposalHome() ) > - mx = opts["root_max"]:0; > - parts = can_mp_reuse( "/", > opts["root_base"]:0, > - mx, parts ); > + tmp = can_home_reuse( 4*1024, 0, parts ); Another magic number why 4kilobytes ( or megabytes???)? what happen if I change it? who set this limits? > + if( size(tmp)>0 ) > + { > + have_home = true; > + parts = tmp; > + } > } > - if( size(parts)>0 ) > + tmp = can_root_reuse( opts["root_base"]:0, > + opts["root_max"]:0, parts > ); > + if( size(tmp)>0 ) > { > - have_home = avail_size > > opts["home_limit"]:0; have_root = true; > - disk["partitions"] = parts; > + parts = tmp; > } > + disk["partitions"] = parts; > y2milestone( "get_inst_proposal reuse have_home > %1 have_root %2", have_home, have_root ); > if( have_home && have_root ) > Josef -- Josef Reidinger Software Engineer Appliance Department SUSE LINUX, s. r. o. Lihovarska 1060/12 190 00 Praha 9 Czech Republic [email protected] SUSE -- To unsubscribe, e-mail: [email protected] To contact the owner, e-mail: [email protected]
