[Gelistirici] fetcher.fetch_url(), file.File.download() ve Bütünlük Kontrolü

[Fatih Aşıcı]

 On Fri, 29 Oct 2010 20:59:13 +0300, Bahadır Kandemir 
 <bahadir at pardus.org.tr> wrote:
> Merhaba,
>
> 1. Bazı yerlerde fetcher.fetch_url(), bazı yerlerde 
> file.File.download()
> kullanılmış. İmzalama işi sırasında epey kafamı karıştırdı bu.
>
> İndirilen dosyayı belirli bir kontrolden geçerse kullanacaksak
> File.download(), kaynak/yama/ek dosya/vs. indireceksek ve 
> bütünlük/imza
> kontrolü yapmayacaksak fetcher.fetch_url() kullanalım gibi bir 
> politika
> belirlesek iyi olur.
>
> 2. PiSi paketlerini ve sıkıştırılmamış paket indekslerini indirirken,
> bütünlük
> kontrolü yapmadan indirip cache/index dizinlerine koyuyoruz. Bu
> yanlış. Dosya
> bütünlük kontrolünü geçene kadar, hedef dizine yazılmaması gerekiyor.
>
> File.download()'da, dosyayı ".tmp" uzantısıyla kayıt eden ve kontrolü
> geçtikten sonra yeniden adlandıran bir değişiklik yaptım, yama ekte.

 Yamayı uygulayıp denemedim. Yorumlarım aşağıda. Unittest'leri
 kırmıyorsa commit edebilirsin.

 Kodun geri kalanı, izlediğimiz kod stiline zaten uymadığı için
 şimdilik stil düzeltmelerini es geçiyorum.

> Index: package.py
> ===================================================================
> --- package.py	(revision 32728)
> +++ package.py	(working copy)
> @@ -23,6 +23,7 @@
>  import pisi.archive as archive
>  import pisi.uri
>  import pisi.metadata
> +import pisi.file
>  import pisi.files
>  import pisi.util as util
>  import fetcher
> @@ -95,7 +96,7 @@
>
>          if not os.path.exists(self.filepath):
>              try:
> -                fetcher.fetch_url(url, dest, ctx.ui.Progress)
> +                pisi.file.File.download(url, dest)
>              except pisi.fetcher.FetchError:
>                  # Bug 3465
>                  if ctx.get_option('reinstall'):
> Index: constants.py
> ===================================================================
> --- constants.py	(revision 32728)
> +++ constants.py	(working copy)
> @@ -57,6 +57,7 @@
>          self.__c.xz_suffix = ".xz"
>
>          self.__c.partial_suffix = ".part"
> +        self.__c.temporary_suffix = ".tmp"
>
>          # suffix for auto generated debug packages
>          self.__c.debug_name_suffix = "-dbginfo"
> Index: file.py
> ===================================================================
> --- file.py	(revision 32728)
> +++ file.py	(working copy)
> @@ -108,13 +108,21 @@
>
>          pisi.util.ensure_dirs(transfer_dir)
>
> +        # Check integrities of all files, unless they are used for 
> checking integrity
> +        check_integrity = not uri.filename().endswith('.sha1sum')
> +
>          if sha1sum:
>              sha1filename = File.download(pisi.uri.URI(uri.get_uri() 
> + '.sha1sum'), transfer_dir)
>              sha1f = file(sha1filename)
>              newsha1 = sha1f.read().split("\n")[0]
>
>          if uri.is_remote_file() or copylocal:
> -            localfile = pisi.util.join_path(transfer_dir, 
> uri.filename())
> +            if check_integrity:
> +                newfile = uri.filename() + 
> ctx.const.temporary_suffix
> +                localfile = pisi.util.join_path(transfer_dir, 
> newfile)
> +            else:
> +                newfile = None
> +                localfile = pisi.util.join_path(transfer_dir, 
> uri.filename())
>

 newfile yerine tmpfile desek daha anlamlı olacak sanki. Bir de 
 aşağıdaki şekilde
 sadeleştirilebilir:

         if uri.is_remote_file() or copylocal:
             tmpfile = check_integrity and uri.filename() + 
 ctx.const.temporary_suffix
             localfile = pisi.util.join_path(transfer_dir, tmpfile or 
 uri.filename())

 Önemli değil; ama olsa iyi olur.

>              # TODO: code to use old .sha1sum file, is this a 
> necessary optimization?
>              #oldsha1fn = localfile + '.sha1sum'
> @@ -128,10 +136,13 @@
>
>              if uri.is_remote_file():
>                  ctx.ui.info(_("Fetching %s") % uri.get_uri(), 
> verbose=True)
> -                pisi.fetcher.fetch_url(uri, transfer_dir, 
> ctx.ui.Progress)
> +                pisi.fetcher.fetch_url(uri, transfer_dir, 
> ctx.ui.Progress, newfile)
>              else:
> -                # copy to transfer dir,
> -                localfile = pisi.util.join_path(transfer_dir, 
> uri.filename())
> +                # copy to transfer dir
> +                if newfile:
> +                    localfile = pisi.util.join_path(transfer_dir, 
> newfile)
> +                else:
> +                    localfile = pisi.util.join_path(transfer_dir, 
> uri.filename())
>                  ctx.ui.info(_("Copying %s to transfer dir") % 
> uri.get_uri(), verbose=True)
>                  shutil.copy(uri.get_uri(), transfer_dir)
>          else:

 Yukarıdakine benzer şekilde:

                 localfile = pisi.util.join_path(transfer_dir, newfile 
 or uri.filename())


> @@ -143,11 +154,26 @@
>                  localfile = pisi.util.join_path(transfer_dir, 
> os.path.basename(localfile))
>                  shutil.copy(oldfn, localfile)
>
> +        def clean_temporary():
> +            corrupt_files = [sha1_file]
> +            if check_integrity:
> +                corrupt_files.append(localfile)

 Listenin adını temp_files yapsak daha iyi olur. Sum dosyası bozuk değil
 aslında.

> +            for filename in corrupt_files:
> +                try:
> +                    os.unlink(filename)
> +                except OSError:
> +                    pass
> +
>          if sha1sum:
>              if (pisi.util.sha1_file(localfile) != newsha1):
> +                clean_temporary()
>                  raise Error(_("File integrity of %s compromised.") % 
> uri)


 fetcher exception verdiğinde de clean_temporary() çağrılacak 
 mı/çağrılmalı mı
 acaba? Fetcher exception'ları yakalanmıyor download() içinde. Eğer 
 exception
 alınırsa bu code path'e hiç girmeyecek.

>
> -        localfile = File.decompress(localfile, compress)
> +        if check_integrity:
> +            shutil.move(os.path.join(transfer_dir, newfile), 
> os.path.join(transfer_dir, uri.filename()))
> +            localfile = File.decompress(os.path.join(transfer_dir, 
> uri.filename()), compress)
> +        else:
> +            localfile = File.decompress(localfile, compress)
>
>          return localfile
>
> Index: fetcher.py
> ===================================================================
> --- fetcher.py	(revision 32728)
> +++ fetcher.py	(working copy)
> @@ -105,19 +105,25 @@
>  class Fetcher:
>      """Fetcher can fetch a file from various sources using various
>      protocols."""
> -    def __init__(self, url, destdir):
> +    def __init__(self, url, destdir, destfile=None):
>          if not isinstance(url, pisi.uri.URI):
>              url = pisi.uri.URI(url)
>
>          if ctx.config.get_option("authinfo"):
>              url.set_auth_info(ctx.config.get_option("authinfo"))
>
> -        self.url      = url
> -        self.destdir  = destdir
> -        self.archive_file = os.path.join(self.destdir, 
> self.url.filename())
> -        self.partial_file = self.archive_file + 
> ctx.const.partial_suffix
> +        self.url = url
> +        self.destdir = destdir
> +        self.destfile = destfile
>          self.progress = None
>
> +        if destfile:
> +            self.archive_file = os.path.join(self.destdir, 
> self.destfile)
> +        else:
> +            self.archive_file = os.path.join(self.destdir, 
> self.url.filename())
> +

 Yerine:

         self.archive_file = os.path.join(destdir, destfile or 
 url.filename())

 olabilir.

> +        self.partial_file = os.path.join(self.destdir, 
> self.url.filename()) + ctx.const.partial_suffix
> +
>          util.ensure_dirs(self.destdir)
>
>      def fetch (self):
> @@ -222,7 +228,7 @@
>
>
>  # helper function
> -def fetch_url(url, destdir, progress=None):
> -    fetch = Fetcher(url, destdir)
> +def fetch_url(url, destdir, progress=None, destfile=None):
> +    fetch = Fetcher(url, destdir, destfile)
>      fetch.progress = progress
>      fetch.fetch()