On 01/09/2011 20:45, Jack Schwartz wrote: > Hi Darren. > > Thanks for your review... > > On 09/ 1/11 02:47 AM, Darren Kenny wrote: >> Hi Jack, >> >> Thanks for looking at this, I have some comments... >> >> General: >> >> - Why is the file called ai_svr4_admin - I think the ai_ should be omitted >> since it's not unique to AI and could just as easily be used in a DC >> build, >> I feel it would suffice to be svr4_admin since it's installed in >> /usr/share/install. > OK. Will change. >> File svr4.py: >> >> - General >> >> It looks like the package datastream may be opened at least 3 times, >> specifically I feel this is excessive if we're using a remote URL: >> >> 1) In get_pkgurl_status() to just see if it's remote or not, and whether >> it's >> a datastream or not - I'm not sure how much is actually downloaded in >> this >> case - if it's small then it may acceptable. > Using snoop, I verified that urllilb2.urlopen() reads about 80K > characters. While significant, it does not seem excessively wasteful in > the grand scheme of things... What do you think?
I think 80K is fine - it's relatively small. >> 2) In ds_pkg_size_and_verify() it's opened again, again to see if it's >> remote >> or not, or a datastream or not >> >> This time it's also fully streamed down from a remote site to see what >> size it is, but we don't save this stream anywhere. > No, I simply read the first few lines until the end of the header. For > most cases this will be only tens of bytes. But still it buffers that 80K. Sounds fine, good to know that it doesn't download the whole file just to get the header info. >> >> 3) Finally, we call the pkgadd command with the URL again, which in turn >> downloads the stream again. > This time the file is read in full. >> >> I really feel that we could do some optimizing here, and only download the >> stream once if it's a remote stream - this could be facilitated by >> a method that encapsulates the urlopen() that does the full streaming (2 >> above) and does it only once caching to a file - subsequent calls would >> open >> the cached file, and the location of this would be what's passed to the > OK. I'll look into reading the remote file myself and passing it to > pkgadd locally, to save those 2 80K reads. I've looked at this code, and I have one problem with it - and that's that the download is being done at the progress estimate stage - this is done very early on in the course of an install - in fact it's done the first time you specify to execute a list of checkpoints that contains this checkpoint. So in most installers, that will mean that you will be doing the download of the package even before target instantiation has been done. Also, since you're possible downloading several MB of file to /system/volatile - which is a RAM disk, it may have a negative effect on the required memory for installation. I'm just not sure there is much to be gained by the installer doing the download itself (based on the figures you've provided above). >> >> >> Maybe there is not any real need to download the whole stream in our code, >> but just look at the HTTP header which should be able to tell you the >> size of >> the datastream - but this may not always be there, so we should also >> handle >> this by returning an 'indeterminate' size. It's only an informational >> message, and we shouldn't really need to download a possibly large file >> just >> to get the information. > I looked into the http way of reading the size, and know how to do > this. However, if one is only installing a single package of a > datastream which has several packages, the size will be grossly > incorrect. This is why I read the datastream header instead. But the HTTP header is more likely to contain the correct size since to even install the smallest package in a datastream, it means you will need to download the whole datastream... The size is only being used to estimate progress - so I believe that the HTTP download size is appropriate to use here. >> >> - lines 116-122: >> >> These might be better placed as part of an if/else is_remote_file at lines >> 126-127 - it's confusing to be opening the remote URL earlier while >> waiting >> to open a local file URL. >> >> (i.e. Looking at line 130, the first question is where is fd being opened >> if >> it's not a local file?) > OK. Will adjust. > > I'll repost another webrev when ready later today, probably after you've > signed off for the day. I'll review the changes separately. Thanks, Darren. > > Thanks again, > Jack >> >> Thanks, >> >> Darren. >> >> >> On 01/09/2011 09:52, Jack Schwartz wrote: >>> Hi everyone. >>> >>> Here is a code review to fix: >>> 7084807<http://monaco.us.oracle.com/detail.jsf?cr=7084807> SVR4 package >>> installs using the new autoinstaller don't work >>> >>> Webrev: >>> https://cr.opensolaris.org/action/browse/caiman/schwartz/7084807_1 >>> >>> Bug report: >>> http://monaco.us.oracle.com/detail.jsf?cr=7084807 >>> >>> Please review and bless by Friday 9/2 10 AM PST. >>> >>> I have done a lot of standalone testing with a special setup. Testing is >>> still >>> in progress and is looking good. I want to get the webrev out before I >>> complete >>> testing so I can get this into B174, pushing Friday COB. I don't >>> anticipate any >>> major changes at this point. >>> >>> Testing has included remote and local datastream files with 1 and>1 packages >>> requested, local directories which are parents to groups of package trees. >>> >>> Testing still to be done: >>> - A full AI install. Up until now I have bypassed IPS package installs to >>> save >>> time. I'll do this first thing in the morning. >>> - Unit tests still need a little more work. >>> >>> Note: I introduce an SVR4 admin file to AI client /usr/share/install >>> directory >>> with this push. It is needed for non-interactive SVR4 installs. >>> >>> Thanks, >>> Jack >>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> [email protected] >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss > > _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
