Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] add a lifetime parameter to Transport::fetch and Repository::read_target #563

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

sunshowers
Copy link
Contributor

@sunshowers sunshowers commented Jan 30, 2023

Looking for feedback on this one -- is this desirable?

Issue #, if available:

Description of changes:

Add a lifetime parameter to Transport::fetch and Repository::read_target.

This is useful in cases where you're implementing a transport that borrows from the reader (e.g. if you're reading a TUF repository from a zip file, returning a ZipFile<'a> as the transport).

This is a breaking change because read_target now borrows from self. I don't think that's a major issue, because in most cases you're going to keep the Repository open for longer than any of the targets you read from it.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…ad_target

This is useful in cases where you're implementing a transport that
borrows from the reader (e.g. if you're treating a zip file as the
reader, returning
[`ZipFile<'a>`](https://docs.rs/zip/latest/zip/read/struct.ZipFile.html)).

This is a breaking change because `read_target` now borrows from `self`.
I don't think that's a major issue, because in most cases you're going
to keep the `Repository` open for longer than any of the files.
@webern
Copy link
Contributor

webern commented Jan 31, 2023

Thank you for this PR. On the surface it makes sense, I'll take a closer look soon.

Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me (to the extent of my ability to reason about lifetimes) and doesn't seem to affect the call sites. Nice!

@zmrow and @iliana

@webern webern requested review from zmrow and ecpullen February 2, 2023 19:36
sunshowers added a commit to sunshowers/omicron that referenced this pull request Feb 2, 2023
Add two commands:

1. `tufaceous archive`, which archives a repository into a zip file.
2. `tufaceous extract`, which extracts a repository from a zip file.

The main reason is that zip supports random access to files and tar does
not.

I was hoping to just be able to read the repository out of the zip file
from memory, but I ran into awslabs/tough#563
while implementing it. Once that is resolved (or we write our own TUF
crate) it should be possible to do that.

Regardless, I don't want to foreclose that option forever, so let's just
use zip for now rather than having to deal with a migration in the
future.

Pull Request: oxidecomputer#2266
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good!

@sunshowers You mentioned this is a breaking change because of the read_target signature change? That's because of the anonymous lifetime?

@sunshowers
Copy link
Contributor Author

sunshowers commented Feb 3, 2023

@zmrow Yes -- the Repository must now stay open longer than any of the readers for the targets read out of it. This is true in common use but I can definitely see realistic users that may have done something like:

  1. Open a Repository.
  2. Create a bunch of readers for targets, and start the reads.
  3. Discard the repository without having completed the reads.

Those users will now have to rearrange their code so that the repository stays open longer than any of the readers.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@webern webern merged commit bc62c75 into awslabs:develop Feb 8, 2023
@sunshowers sunshowers deleted the lifetime-param branch February 15, 2023 18:58
sunshowers added a commit to oxidecomputer/omicron that referenced this pull request Feb 15, 2023
Add two commands:

1. `tufaceous archive`, which archives a repository into a zip file.
2. `tufaceous extract`, which extracts a repository from a zip file.

## Why zip and not tar?

The main reason is that zip supports random access to files and tar does
not.

I was hoping to just be able to read the repository out of the zip file
from memory, but I ran into awslabs/tough#563
while implementing it. Once that is resolved (or we write our own TUF
crate) it should be possible to do that.

Regardless, I don't want to foreclose that option forever, so let's just
use zip for now rather than having to deal with a migration in the
future.

Depends on #2300.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants