Re: 05/07: inferior: Fix concurrent cached-profile calls.

Re: 05/07: inferior: Fix concurrent cached-profile calls.

[Ludovic Courtès]

Hi Mathieu,

guix-commits@gnu.org skribis:

> commit 6ee7e3d26b8f5d2a234518cc2ab1bfeba7cd7c18
> Author: Mathieu Othacehe <othacehe@gnu.org>
> AuthorDate: Fri Mar 5 12:49:06 2021 +0100
>
>     inferior: Fix concurrent cached-profile calls.
>     
>     * guix/inferior.scm (cached-profile): Do not create the profile symlink if it
>     already exists.

[...]

> +++ b/guix/inferior.scm
> @@ -755,8 +755,9 @@ seconds.  This procedure opens a new connection to the build daemon."
>              (built-derivations (list profile))
>              ;; Note: Caching is fine even when AUTHENTICATE? is false because
>              ;; we always call 'latest-channel-instances?'.
> -            (symlink* (derivation->output-path profile) cached)
> -            (add-indirect-root* cached)
> +            (unless (file-exists? cached)
> +              (symlink* (derivation->output-path profile) cached)
> +              (add-indirect-root* cached))
>              (return cached))))))

It should be ‘munless’ instead of ‘unless’ since we’re in an ‘mbegin’.

However, there’s already a (file-exists? cached) call a few lines
above.  So what you need instead is a TOCTTOU-free ‘symlink’, along
these lines:

  (define (symlink/safe old new)
    (catch 'system-error
      (lambda ()
        (symlink old new))
      (lambda args
        (unless (= EEXIST (system-error-errno args))
          (apply throw args)))))

  ;; …

  (define symlink*
    (lift2 symlink/safe %store-monad))

WDYT?

Ludo’.

Re: 04/07: inferior: Break cached-channel-instance into two procedures.

[Ludovic Courtès]

Hi Mathieu!

guix-commits@gnu.org skribis:

> commit 7d63b775513e7049047222dbe403a4181f63828d
> Author: Mathieu Othacehe <othacehe@gnu.org>
> AuthorDate: Fri Mar 5 09:51:42 2021 +0100
>
>     inferior: Break cached-channel-instance into two procedures.
>     
>     Break cached-channel-instance into two different procedures:
>     channels->cached-profile and instances->cached-profile operating respectively
>     on channels and channels instances.
>     
>     * guix/inferior.scm (cached-channel-instance): Rename it into ...
>     (cached-profile): ... this new procedure.
>     (channels->cached-profile, instances->cached-profile): New procedures.
>     * guix/scripts/time-machine.scm (guix-time-machine): Adapt accordingly.

[...]

> -(define* (cached-channel-instance store
> -                                  channels
> -                                  #:key
> -                                  (authenticate? #t)
> -                                  (cache-directory (%inferior-cache-directory))
> -                                  (ttl (* 3600 24 30)))
> -  "Return a directory containing a guix filetree defined by CHANNELS, a list of channels.
> -The directory is a subdirectory of CACHE-DIRECTORY, where entries can be reclaimed after TTL seconds.
> -This procedure opens a new connection to the build daemon.  AUTHENTICATE?
> -determines whether CHANNELS are authenticated."
> -  (define commits
> -    ;; Since computing the instances of CHANNELS is I/O-intensive, use a
> -    ;; cheaper way to get the commit list of CHANNELS.  This limits overhead
> -    ;; to the minimum in case of a cache hit.
> -    (map channel-full-commit channels))
> -
> +(define* (cached-profile store instances
> +                         #:key
> +                         cache-directory
> +                         commits ttl)
> +  "Return a directory containing a guix filetree defined by INSTANCES, a
> +procedure returning a list of channel instances.  The directory is a
> +subdirectory of CACHE-DIRECTORY, where entries can be reclaimed after TTL
> +seconds.  This procedure opens a new connection to the build daemon."

We need to keep ‘cached-channel-instance’ because it’s part of the
public API and used outside Guix (in Guix-Jupyter at least).  We can use
‘define-deprecated’.

Also, I think ‘cached-profile’ doesn’t quite capture what this is
about.  :-)  The docstring should mention what COMMITS is.  The fact
that INSTANCES is a thunk is a bit awkward and IMO not great for a
public interface.

WDYT?

Thanks,
Ludo’.

Re: 05/07: inferior: Fix concurrent cached-profile calls.

[Mathieu Othacehe]

Hey Ludo,

> However, there’s already a (file-exists? cached) call a few lines
> above.  So what you need instead is a TOCTTOU-free ‘symlink’, along
> these lines:
>
>   (define (symlink/safe old new)
>     (catch 'system-error
>       (lambda ()
>         (symlink old new))
>       (lambda args
>         (unless (= EEXIST (system-error-errno args))
>           (apply throw args)))))
>
>   ;; …
>
>   (define symlink*
>     (lift2 symlink/safe %store-monad))
>
> WDYT?

Fixed with a831ff6bc3f92ab4ecf6135e4d6386f14189ad06.

Thanks,

Mathieu

Re: 04/07: inferior: Break cached-channel-instance into two procedures.

[Mathieu Othacehe]

Hey,

> We need to keep ‘cached-channel-instance’ because it’s part of the
> public API and used outside Guix (in Guix-Jupyter at least).  We can use
> ‘define-deprecated’.
>
> Also, I think ‘cached-profile’ doesn’t quite capture what this is
> about.  :-)  The docstring should mention what COMMITS is.  The fact
> that INSTANCES is a thunk is a bit awkward and IMO not great for a
> public interface.
>
> WDYT?

Yeah you're right the abstraction is really not great. I'll tweak
Cuirass to use the existing API instead.

Reverted with: 8898eaec57f6294221888e6dca1802abdd3d5868.

Thanks,

Mathieu