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

client/file: *File.ReadAt doesn't return io.EOF when file is read to the end #648

Closed
rschio opened this issue Jul 8, 2023 · 1 comment · Fixed by #649
Closed

client/file: *File.ReadAt doesn't return io.EOF when file is read to the end #648

rschio opened this issue Jul 8, 2023 · 1 comment · Fixed by #649
Assignees

Comments

@rschio
Copy link

rschio commented Jul 8, 2023

*File.ReadAt() returns nil error when the file is read to the end and doesn't completly fill the []byte it is reading to.

I expected to see io.EOF.

According to the io.ReaderAt interface:

When ReadAt returns n < len(p), it returns a non-nil error
explaining why more bytes were not returned. In this respect,
ReadAt is stricter than Read.

Here is a test explaning better:

package main

import (
	"io"
	"log"
	"testing"

	"upspin.io/client"
	"upspin.io/config"
	"upspin.io/path"
	"upspin.io/transports"
	"upspin.io/upbox"
	"upspin.io/upspin"
)

func TestReadAt(t *testing.T) {
	c, cfg, cleanup := newClient()
	t.Cleanup(cleanup)

	root := upspin.PathName(cfg.UserName())
	if _, err := c.MakeDirectory(root); err != nil {
		t.Fatalf("failed to create root dir: %v", err)
	}

	name := path.Join(root, "file.txt")
	data := []byte("hello")
	if _, err := c.Put(name, data); err != nil {
		t.Fatalf("failed to create file.txt: %v", err)
	}

	f, err := c.Open(name)
	if err != nil {
		t.Fatalf("failed to open file: %v", err)
	}
	defer f.Close()

	// When ReadAt returns n < len(p), it returns a non-nil error
	// explaining why more bytes were not returned. In this respect,
	// ReadAt is stricter than Read.
	p := make([]byte, len(data))
	n, err := f.ReadAt(p, 1)
	if n < len(p) && err == nil {
		t.Fatalf("should return (%d, io.EOF), got (%d, %v)", len(data)-1, n, err)
	}
}

const schema = `
users:
  - name: user

servers:
  - name: keyserver
  - name: storeserver
  - name: dirserver

domain: example.com
`

func newClient() (upspin.Client, upspin.Config, func()) {
	sc, err := upbox.SchemaFromYAML(schema)
	if err != nil {
		log.Fatalf("failed to parse schema: %v", err)
	}

	if err := sc.Start(); err != nil {
		log.Fatalf("failed to start schema: %v", err)
	}
	cleanup := func() { sc.Stop() }

	cfg, err := config.FromFile(sc.Config(sc.Users[0].Name))
	if err != nil {
		cleanup()
		log.Fatalf("failed to parse config: %v", err)
	}

	transports.Init(cfg)
	c := client.New(cfg)

	return c, cfg, cleanup
}
@n2vi n2vi self-assigned this Jul 14, 2023
n2vi added a commit that referenced this issue Jul 14, 2023
When ReadAt returns fewer bytes than requested, it is supposed to
return io.EOF rather than nil.

Fixes #648.
@n2vi n2vi reopened this Jul 15, 2023
n2vi added a commit that referenced this issue Jul 16, 2023
As noted in #648, the ReadAt implementation in client/file/file.go
should return err io.EOF upon short read. This change adds a test
to warn of that bug.

But currently we can't run "go test" in client/ because TestFileSeek
times out. Ideally we'd fix the test, but that will require a lot of
discussion. For now, just add a provision to skip via "go test -short".
Issue #654 filed.
@n2vi
Copy link
Contributor

n2vi commented Jul 16, 2023

Rodrigo,

Given my inexperience with our new review system, it seems I can't add you as an official reviewer for #655 but I would welcome your comments there before I ask Rob to review.

Thanks, Eric

@n2vi n2vi closed this as completed in 95c579d Jul 16, 2023
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 a pull request may close this issue.

2 participants