RDB: style enhancement for rdb load

[mapleFU]

After review #1798 , the patch is great, here is just some style enhancement:

1. RDBStream::Read returns StatusOr<size_t>, but if status is ok, the size is always equal to input, so change it to return Status
2. Some move rather than copy

[mapleFU]

I wonder if it's a RdbStringStream, it might call memcpy on std::vector<char>, and when size is 0, maybe memcpy will memcpy 0 in a invalid address, which might be a ub?

[mapleFU]

This is a const_cast but I think it's ok to calling it?

[mapleFU]

History:
 - Download (curl) error for 'http://mirrorcache-us.opensuse.org/update/leap/15.5/backports/repodata/6693d5099c437a8a40fc4c4d94d4916f050d24e533507f4e8a3360ba1d2709e3-deltainfo.xml.gz':
   Error code: Connection failed
   Error message: Failed to connect to mirrorcache-us.opensuse.org port 80 after 0 ms: Couldn't connect to server
 - Can't provide ./repodata/6693d5099c437a8a40fc4c4d94d49[16](https://github.com/apache/kvrocks/actions/runs/6576647903/job/17867181773?pr=1839#step:5:17)f050d24e533507f4e8a3360ba1d2709e3-deltainfo.xml.gz

Hmmm

[PragmaTwice]

A good change! But I think the real situation can be more complex, e.g. if eof is reached do we need to use istream::gcount to access the real position?

[mapleFU]

Yes, you're right, let me update this.

[mapleFU]

ifs_.read(buf, static_cast<std::streamsize>(read_bytes));
    if (!ifs_.good()) {
      if (!ifs_.eof()) {
        return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))};
      }
      auto eof_read_bytes = static_cast<size_t>(ifs_.gcount());
      if (read_bytes != eof_read_bytes) {
        return {Status::NotOK, fmt::format("read failed: {}:", strerror(errno))};
      }
    }

I've update it to code like this. In read(2), if file reaches EOF, the read would return 0. And return -1 when meets error. So I think previous code is ok. But I don't get what the ifstream works even after I gothrough the cppreference. So I change it to the code above.

[PragmaTwice]

After a deep look into this function, I think it need some other change. In my view, I think it should not just return error when facing EOF.

StatusOr<size_t> Read(char *dst, size_t len) {
    auto l = std::min(len, real_len);
    copy(src, dst, l);
    return l;
}

[PragmaTwice]

The error should be returned only when some IO failure occurs, rather than not enough length.

[mapleFU]

@PragmaTwice The api is designed like this. It's not a standard fs api.

I've checkout redis rdb impl in rio, it just provide the same syntax.

[mapleFU]

For comparing:

static inline size_t rioRead(rio *r, void *buf, size_t len) {
    if (r->flags & RIO_FLAG_READ_ERROR) return 0;
    while (len) {
        size_t bytes_to_read = (r->max_processing_chunk && r->max_processing_chunk < len) ? r->max_processing_chunk : len;
        if (r->read(r,buf,bytes_to_read) == 0) {
            r->flags |= RIO_FLAG_READ_ERROR;
            return 0;
        }
        if (r->update_cksum) r->update_cksum(r,buf,bytes_to_read);
        buf = (char*)buf + bytes_to_read;
        len -= bytes_to_read;
        r->processed_bytes += bytes_to_read;
    }
    return 1;
}

[mapleFU]

I think if we require a fs api, we can wrap a

StatusOr<size_t> Read(buf, size_t);
StatusOr<Buffer> Read(..);
StatusOr<Buffer> ReadAt(..);

Style api as underlying impl, but this case I think rdb will gurantee the size should match.

[PragmaTwice]

I see. LGTM.