[pbs-devel] [PATCH proxmox-backup v3 1/2] tape: changer: correctly consume data in decode_element_status_page

[pbs-devel] [PATCH proxmox-backup v3 1/2] tape: changer: correctly consume data in decode_element_status_page

[Dominik Csapak]

instead of 'blindly' trusting the changer to deliver the fields written
in the specification, trust the length data it returns in the header.

we prepare the reader to be exaclty the size of the descriptor given
to us by the subheader, and offset the descriptor data by exactly
that amount after each one

we can drop the reading of the 'reserved' bytes, since we do not need
them or the data after

this also makes the code to read the rest of the page obsolete,
since the we simply start on the correct offset anyway

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v2:
* drop 'sg_pt' from commit subject
* drop the 'remaining_size' calculation, instead prepare the reader
  to be at the right offset

 src/tape/changer/sg_pt_changer.rs | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index a15d8192..ddbb5d74 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -651,17 +651,18 @@ fn decode_element_status_page(
             }
 
             let descr_data = reader.read_exact_allocated(len)?;
-            let mut reader = &descr_data[..];
+            let mut descriptors_to_read = &descr_data[..];
+            let descriptor_len = subhead.descriptor_length as usize;
 
             loop {
-                if reader.is_empty() {
+                if descriptors_to_read.is_empty() {
                     break;
                 }
-                if reader.len() < (subhead.descriptor_length as usize) {
+                if descriptors_to_read.len() < descriptor_len {
                     break;
                 }
 
-                let len_before = reader.len();
+                let mut reader = &descriptors_to_read[..descriptor_len];
 
                 match subhead.element_type_code {
                     1 => {
@@ -672,9 +673,6 @@ fn decode_element_status_page(
 
                         subhead.skip_alternate_volume_tag(&mut reader)?;
 
-                        let mut reserved = [0u8; 4];
-                        reader.read_exact(&mut reserved)?;
-
                         result.last_element_address = Some(desc.element_address);
 
                         let status = TransportElementStatus {
@@ -691,9 +689,6 @@ fn decode_element_status_page(
 
                         subhead.skip_alternate_volume_tag(&mut reader)?;
 
-                        let mut reserved = [0u8; 4];
-                        reader.read_exact(&mut reserved)?;
-
                         result.last_element_address = Some(desc.element_address);
 
                         if subhead.element_type_code == 3 {
@@ -764,18 +759,8 @@ fn decode_element_status_page(
                     code => bail!("got unknown element type code {}", code),
                 }
 
-                // we have to consume the whole descriptor size, else
-                // our position in the reader is not correct
-                let len_after = reader.len();
-                let have_read = len_before - len_after;
-                let desc_len = subhead.descriptor_length as usize;
-                if desc_len > have_read {
-                    let mut left_to_read = desc_len - have_read;
-                    if left_to_read > len_after {
-                        left_to_read = len_after; // reader has not enough data?
-                    }
-                    let _ = reader.read_exact_allocated(left_to_read)?;
-                }
+                // set position to next descriptor
+                descriptors_to_read = &descriptors_to_read[descriptor_len..];
             }
         }
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 2/2] tape: changer: improve error message on wrong counts

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tape/changer/sg_pt_changer.rs | 37 ++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index ddbb5d74..6abaac99 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -421,17 +421,38 @@ pub fn read_element_status<F: AsRawFd>(file: &mut F) -> Result<MtxStatus, Error>
     let page = get_element(&inquiry, &mut sg_raw, ElementType::MediumTransport, allocation_len, false)?;
     transports.extend(page.transports);
 
-    if (setup.transport_element_count as usize) != transports.len() {
-        bail!("got wrong number of transport elements");
+    let transport_count = setup.transport_element_count as usize;
+    let storage_count = setup.storage_element_count as usize;
+    let import_export_count = setup.import_export_element_count as usize;
+    let transfer_count = setup.transfer_element_count as usize;
+
+    if transport_count != transports.len() {
+        bail!(
+            "got wrong number of transport elements: expoected {}, got{}",
+            transport_count,
+            transports.len()
+        );
     }
-    if (setup.storage_element_count as usize) != storage_slots.len() {
-        bail!("got wrong number of storage elements");
+    if storage_count != storage_slots.len() {
+        bail!(
+            "got wrong number of storage elements: expected {}, got {}",
+            storage_count,
+            storage_slots.len(),
+        );
     }
-    if (setup.import_export_element_count as usize) != import_export_slots.len() {
-        bail!("got wrong number of import/export elements");
+    if import_export_count != import_export_slots.len() {
+        bail!(
+            "got wrong number of import/export elements: expected {}, got {}",
+            import_export_count,
+            import_export_slots.len(),
+        );
     }
-    if (setup.transfer_element_count as usize) != drives.len() {
-        bail!("got wrong number of transfer elements");
+    if transfer_count != drives.len() {
+        bail!(
+            "got wrong number of transfer elements: expected {}, got {}",
+            transfer_count,
+            drives.len(),
+        );
     }
 
     // create same virtual slot order as mtx(1)
-- 
2.30.2