Implement downloading of remote gz images #34

Razaloc · 2022-10-11T12:51:50Z

Allow bmap-rs to copy images from a remote file:

Accept also an HTTP/HTTPS url argument
Convert setup_input into two different functions to handle the input: a new one setup_remote_input and the previous one as setup_local_input
By now it error if the downloaded file is not a .gz file as "not implemented error"
Adapt code to the async case.

Closes: #9
Closes: #8

Signed-off-by: Rafael Garcia Ruiz rafael.garcia@collabora.com

This is a preparation to allow bmap-rs fetch files, so it needs to accept urls as well as local image paths. At this point the url will be accepted but the file wont be downloaded causing it to not be able to find it. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc · 2022-10-11T12:52:36Z

WIP

setup_input turned into two different functions setup_local_input and setup_remote_input which is still to be completed since it needs async enviroment. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

When using a remote image filter if the compression type isn't ".gz" the only one support by now. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

bmap-rs/src/main.rs

setup_local_input and setup_remote_input now return a Input enum that is used to switch between remote or local image origin. The download is implemented so the file is divided in pieces and copied part by part. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

As with remote source the file to copy and the bmap are not in the same folder we need to adapt the path, in this case url to find the bmap file. It is supposed to be in the folder we are executing bmap-rs and have same name plus ".bmap" Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

bmap::copy_async emulates the use of bmap::copy but adapting to async cases. A new trait AsyncSeekForward is needed to substitute SeekForward in those cases. This duplicity is meant to be reduced in the future. Some new dependencies were needed. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

bmap::copy_async needs inputs with different traits than bmap::copy WIP Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Get some help from Kov figuring out things about async traits with rust while using read and write functions. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

bmap-rs/src/main.rs

bmap-rs/Cargo.toml

Adapt traits implementations to async. Add https support. Change the loop of data chunks to a stream. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

When writting the buf need the index range of the reader. Fix some tipos and clippy warnings Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Add index range into the reading buf for the async copy function to avoid overflow. Fix typo. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

When downloading the image from a remote source the bmap will also be searched remotly and downloaded. The bmap url will be same as image url but changing the extension with ".bmap" bmap/src/lib.rs formated with rustfmt Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

bmap-rs/src/main.rs

Now the bmap data is kept in memory inside a Cursor instead than in a File Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

sjoerdsimons · 2022-10-22T09:47:33Z

Can this be split out in multiple MR's building up topics in small steps (e.g. building up the async support in one MR, bugfixes for existing code in another) and also please do a clean rebase and squashing where it makes sense.

sjoerdsimons · 2022-10-22T09:51:19Z

bmap-rs/src/main.rs

+async fn fetch_url_http(url: hyper::Uri) -> Result<Response<Body>, hyper::Error> {
+    let client = Client::new();
+    client.get(url.clone()).await
+}
+async fn fetch_url_https(url: hyper::Uri) -> Result<Response<Body>, hyper::Error> {
+    let https = HttpsConnector::new();
+    let client = Client::builder().build::<_, hyper::Body>(https);
+    client.get(url).await
+}
+async fn setup_remote_input(url: Uri, fpath: &Path) -> Result<Input> {
+    if fpath.extension().unwrap() != "gz" {
+        bail!("Image file format not implemented")
+    }
+    let input = Input::Remote(match url.scheme_str() {
+        Some("https") => fetch_url_https(url).await?,
+        Some("http") => fetch_url_http(url).await?,
+        _ => {
+            bail!("url is not http or https")
+        }
+    });
+    Ok(input)
+}


Please switch to using reqwest rather then Hyper; There is no reason for a tool like bmap to use a low-level http library like hyper directly. In particular that would avoid this sillyness to support multiple schemes.

sjoerdsimons · 2022-10-22T10:12:07Z

bmap-rs/src/main.rs

+            let mut c = Cursor::new(Vec::new());
+            while let Some(next) = bmap_res.data().await {
+                let chunk = next?;
+                c.write_all(&chunk)?;
+            }
+            c.set_position(0);
+            let mut xml = String::new();
+            c.read_to_string(&mut xml)?;


Ignoring the fact you onjly need to solve this due to using Hyper; You don't want to use a cursor here or do this loop by hand, hyper has its hyper::body::to_bytes helper that collects everything already; followed by std::str::from_utf8

sjoerdsimons · 2022-10-22T10:14:37Z

bmap-rs/src/main.rs

+            let mut output = tokio::fs::OpenOptions::new()
+                .write(true)
+                .create(true)
+                .open(c.dest)
+                .await?;
+            ftruncate(output.as_raw_fd(), bmap.image_size() as i64)
+                .context("Failed to truncate file")?;


This should be done outside of the input_type determination as it's not dependant on the input

sjoerdsimons · 2022-10-22T10:17:30Z

bmap-rs/src/main.rs

+            bmap::copy_async(&mut chunk, &mut output, &bmap).await?;
+            println!("Done: Syncing...");
+            output.sync_all().await.expect("Sync failure");


This should probably also be generic ; There is no reason why local files should be sync and remote files async, they can be treated in the same way.

sjoerdsimons · 2022-10-22T10:20:10Z

bmap/src/discarder.rs

+        let mut left = forward as usize;
+        while left > 0 {
+            let toread = left.min(buf.len());
+            let r = block_on(self.read(&mut buf[0..toread]))?;


don't call block_on in an async method; as that starts a new executors that blocks the overall executor; Instead do a proper async read

right so that explains why in my testing CPU usage would peak quite high

Razaloc · 2022-10-24T17:41:30Z

#41 Takes the essential for the download feature, apply the use of reqwest and avoid using cursor.
I've left the finding remote bmap for next PR.

Razaloc · 2022-10-24T18:56:19Z

#42 Is a small fix to adapt the version numbers to dependabot.

Razaloc force-pushed the wip/rafaelgarrui/download branch 2 times, most recently from 3185134 to bda5a15 Compare October 11, 2022 14:48

Razaloc added 2 commits October 11, 2022 17:14

Handle remote or local input differently

8c375a7

setup_input turned into two different functions setup_local_input and setup_remote_input which is still to be completed since it needs async enviroment. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Filter non supported compression files

0218228

When using a remote image filter if the compression type isn't ".gz" the only one support by now. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc force-pushed the wip/rafaelgarrui/download branch from bda5a15 to 0218228 Compare October 11, 2022 15:17

obbardc requested changes Oct 11, 2022

View reviewed changes

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

Razaloc force-pushed the wip/rafaelgarrui/download branch from 476fc73 to 45cdefc Compare October 11, 2022 19:41

Razaloc added 2 commits October 12, 2022 17:45

Razaloc force-pushed the wip/rafaelgarrui/download branch 6 times, most recently from 219f105 to 402a111 Compare October 13, 2022 16:23

Adapt the input and output for bmap::copy_async

99d7227

bmap::copy_async needs inputs with different traits than bmap::copy WIP Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc force-pushed the wip/rafaelgarrui/download branch from 402a111 to 99d7227 Compare October 13, 2022 19:09

Help from Kov

b8c280d

Get some help from Kov figuring out things about async traits with rust while using read and write functions. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc force-pushed the wip/rafaelgarrui/download branch from bb42bdb to db873d2 Compare October 19, 2022 17:40

obbardc reviewed Oct 20, 2022

View reviewed changes

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

obbardc reviewed Oct 20, 2022

View reviewed changes

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

obbardc reviewed Oct 20, 2022

View reviewed changes

bmap-rs/Cargo.toml Outdated Show resolved Hide resolved

Razaloc force-pushed the wip/rafaelgarrui/download branch 5 times, most recently from ab8ec0e to 9867819 Compare October 20, 2022 14:37

Fixing async envroment for downloading gzfile/copy

a49e473

Adapt traits implementations to async. Add https support. Change the loop of data chunks to a stream. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc force-pushed the wip/rafaelgarrui/download branch from 9867819 to a49e473 Compare October 20, 2022 15:18

Fix Checksum problem

b3c9900

When writting the buf need the index range of the reader. Fix some tipos and clippy warnings Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc marked this pull request as ready for review October 21, 2022 11:51

Fix overflowing in bmap::copy_async

ca8ea60

Add index range into the reading buf for the async copy function to avoid overflow. Fix typo. Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Razaloc force-pushed the wip/rafaelgarrui/download branch from b270235 to ca8ea60 Compare October 21, 2022 13:26

obbardc reviewed Oct 21, 2022

View reviewed changes

bmap-rs/src/main.rs Outdated Show resolved Hide resolved

Razaloc added 2 commits October 21, 2022 19:29

Download from remote bmap dont create new file

7362875

Now the bmap data is kept in memory inside a Cursor instead than in a File Signed-off-by: Rafael Garcia Ruiz <rafael.garcia@collabora.com>

Merge branch 'main' into wip/rafaelgarrui/download

51cfed1

sjoerdsimons requested changes Oct 22, 2022

View reviewed changes

This was referenced Oct 24, 2022

Download compressed gz image from remote source #41

Merged

Adapt dependencies version numbers #42

Merged

Razaloc closed this Nov 25, 2022

Razaloc deleted the wip/rafaelgarrui/download branch December 1, 2022 10:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement downloading of remote gz images #34

Implement downloading of remote gz images #34

Razaloc commented Oct 11, 2022 •

edited

Razaloc commented Oct 11, 2022

sjoerdsimons commented Oct 22, 2022

sjoerdsimons Oct 22, 2022 •

edited

sjoerdsimons Oct 22, 2022

sjoerdsimons Oct 22, 2022

sjoerdsimons Oct 22, 2022

sjoerdsimons Oct 22, 2022

obbardc Oct 24, 2022

Razaloc commented Oct 24, 2022 •

edited

Razaloc commented Oct 24, 2022

Implement downloading of remote gz images #34

Implement downloading of remote gz images #34

Conversation

Razaloc commented Oct 11, 2022 • edited

Razaloc commented Oct 11, 2022

sjoerdsimons commented Oct 22, 2022

sjoerdsimons Oct 22, 2022 • edited

Choose a reason for hiding this comment

sjoerdsimons Oct 22, 2022

Choose a reason for hiding this comment

sjoerdsimons Oct 22, 2022

Choose a reason for hiding this comment

sjoerdsimons Oct 22, 2022

Choose a reason for hiding this comment

sjoerdsimons Oct 22, 2022

Choose a reason for hiding this comment

obbardc Oct 24, 2022

Choose a reason for hiding this comment

Razaloc commented Oct 24, 2022 • edited

Razaloc commented Oct 24, 2022

Razaloc commented Oct 11, 2022 •

edited

sjoerdsimons Oct 22, 2022 •

edited

Razaloc commented Oct 24, 2022 •

edited