Skip to content

feat: initial commit for erofs support#14

Closed
rchincha wants to merge 2 commits intoproject-machine:mainfrom
rchincha:erofs
Closed

feat: initial commit for erofs support#14
rchincha wants to merge 2 commits intoproject-machine:mainfrom
rchincha:erofs

Conversation

@rchincha
Copy link
Copy Markdown
Contributor

@rchincha rchincha commented Sep 6, 2024

No description provided.

@rchincha rchincha force-pushed the erofs branch 3 times, most recently from 0abac16 to 2843a67 Compare September 30, 2024 21:37
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 30, 2024

Codecov Report

Attention: Patch coverage is 16.44833% with 574 lines in your changes missing coverage. Please review.

Project coverage is 16.28%. Comparing base (ad93e06) to head (c01a838).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
erofs/erofs.go 7.48% 363 Missing and 8 partials ⚠️
erofs/verity.go 20.74% 185 Missing and 6 partials ⚠️
erofs/superblock.go 77.77% 4 Missing and 4 partials ⚠️
erofs/mediatype.go 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #14      +/-   ##
==========================================
- Coverage   16.52%   16.28%   -0.25%     
==========================================
  Files           6       10       +4     
  Lines         932     1640     +708     
==========================================
+ Hits          154      267     +113     
- Misses        757     1334     +577     
- Partials       21       39      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>
@rchincha rchincha force-pushed the erofs branch 2 times, most recently from c27c15d to 29321f8 Compare October 3, 2024 21:06
We are going to support multiple underlying filesystems (squash, erofs
and maybe more). As long as they are filesystem image blobs, verity data
can be appended.

Signed-off-by: Ramkumar Chinchani <rchincha@cisco.com>
Comment thread erofs/erofs.go
type erofsFuseInfoStruct struct {
Path string
Version string
SupportsNotfiy bool
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This typo is consistent but will be murder to maintain later :)

Comment thread erofs/erofs.go
// everyting underneath is also implicitly excluded. The
// AddExclude()/AddInclude() methods do the math to figure out what is the
// correct set of things to exclude or include based on what paths have been
// previously included or excluded.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm finding this hard to reason about, and therefore finding it hard to figure out whether AddExclude() is doing the right thing.

I think you're mainly thinking in terms of "I am generating a changeset and therefore want to include only files which have changed", is that right?

Comment thread erofs/erofs.go
// If /usr/bin/ls has changed but /usr hasn't, we don't want to list
// /usr in the include paths any more, so let's be sure to only
// add things which aren't prefixes.
if strings.HasPrefix(inc, p) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But what if both /usr/bin/cal and /usr/bin/calendar have changed? Will this check end up possibly ignoring /usr/bin/cal?

Comment thread erofs/erofs.go
eps.include = append(eps.include, orig)
}

func (eps *ExcludePaths) String() (string, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I assume you are using this only to spit out a file to feed to mkerofs. But because it is generally called String(), I would expect it to be more generally useful, so in particular would expect it to also print the includes.

I'm not sure the best way to handle this. Probably best to just leave a comment above it explaining that some information does get lost and why.

Comment thread erofs/erofs.go
return blob, GenerateErofsMediaType(compression, verity), rootHash, nil
}

func isMountedAtDir(_, dest string) (bool, error) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is checking that the source is correct a TODO?

@rchincha
Copy link
Copy Markdown
Contributor Author

PR #25 supersedes this one, so closing.

@rchincha rchincha closed this Nov 10, 2024
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.

2 participants