Mailinglist Archive: yast-devel (15 mails)

< Previous Next >
[yast-devel] Re: [yast-commit] r67038 - in /trunk/storage: package/yast2-storage.changes storage/src/modules/StorageProposal.ycp
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
fehr@xxxxxxxxxxxxxxxxx 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 - fehr@xxxxxxx +
+- fix proposal to reuse also larger root filesystems (bnc#727362)
+
+-------------------------------------------------------------------
Tue Dec 06 14:08:41 CET 2011 - aschnell@xxxxxxx

- 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

jreidinger@xxxxxxxx
SUSE
--
To unsubscribe, e-mail: yast-devel+unsubscribe@xxxxxxxxxxxx
To contact the owner, e-mail: yast-devel+owner@xxxxxxxxxxxx

< Previous Next >
This Thread
  • No further messages