Page MenuHomePhabricator

[services] Backup - Blob Put Client - Update put API
ClosedPublic

Authored by karol on Sep 1 2022, 4:06 AM.
Tags
None
Referenced Files
F3276255: D5004.id16436.diff
Sat, Nov 16, 7:25 AM
Unknown Object (File)
Sun, Nov 10, 6:58 AM
Unknown Object (File)
Sun, Oct 27, 8:10 PM
Unknown Object (File)
Sun, Oct 27, 8:10 PM
Unknown Object (File)
Sun, Oct 27, 8:10 PM
Unknown Object (File)
Sun, Oct 27, 8:10 PM
Unknown Object (File)
Sun, Oct 27, 8:10 PM
Unknown Object (File)
Sun, Oct 27, 8:08 PM

Details

Summary

Depends on D5003

We want to use rust's Results so we can catch errors in c++.

Test Plan

backup builds

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

karol edited the test plan for this revision. (Show Details)
karol added reviewers: tomek, varun, max.
karol edited the test plan for this revision. (Show Details)
tomek added inline comments.
services/backup/blob_client/src/lib.rs
13 ↗(On Diff #16164)

Do you think this comment is useful? For me, if names start with put it is obvious that these are a part of put, but feel free to keep it if it's useful for you.

15 ↗(On Diff #16164)

It would be great if we could avoid having unsafe at function level, so that safe code could call it. I prefer having unsafe sections as small as possible, so maybe we can have an unsafe block in the function instead of the whole unsafe function?

This revision is now accepted and ready to land.Sep 5 2022, 2:13 AM
services/backup/blob_client/src/lib.rs
13 ↗(On Diff #16164)

For me, it looks better once there's also the get section, but I can remove this, not a problem.

15 ↗(On Diff #16164)

Rust complains:

pointer argument requires that the function be marked unsafe

I assume we have to mark these functions unsafe. I don't fully understand your concern, they're about to be called from c++ exclusively and I didn't notice any impact of unsafe on how we call them in c++ really. And I don't think we'll want to use them from rust.

services/backup/blob_client/src/lib.rs
15 ↗(On Diff #16164)

Ok, that surprising. I thought that only dereferencing a raw pointer is unsafe.
https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html#unsafe-superpowers
So maybe what happens here is that Rust automatically dereferences a raw pointer when it is a function argument...

I don't fully understand your concern, they're about to be called from c++ exclusively and I didn't notice any impact of unsafe on how we call them in c++ really.

The issue is not on C++ side but on the Rust one. If we mark the function as unsafe, we can use unsafe operations in the whole function. According to the book:

People are fallible, and mistakes will happen, but by requiring these five unsafe operations to be inside blocks annotated with unsafe you’ll know that any errors related to memory safety must be within an unsafe block. Keep unsafe blocks small; you’ll be thankful later when you investigate memory bugs.

It is a good practice to keep unsafe blocks as small as possible.

services/backup/blob_client/src/lib.rs
15 ↗(On Diff #16164)

Discussed offline. I understand that it's better to make unsafe areas as small as possible but here we have no choice, I'm afraid.