[pbs-devel] [PATCH proxmox-backup v2 1/6] api: types: CHANGER_DRIVENUM_SCHEMA: increase maximum drives per changer

[pbs-devel] [PATCH proxmox-backup v2 1/6] api: types: CHANGER_DRIVENUM_SCHEMA: increase maximum drives per changer

[Dominik Csapak]

to 255. 8 drives per changer was a rather arbitrary limitation and could
well be reached in practice with big libraries.

Altough 255 is still a arbirtrary limitation, this is much less likely
to be reached in practice.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no change in v2
 src/api2/types/tape/drive.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/api2/types/tape/drive.rs b/src/api2/types/tape/drive.rs
index ecc65088..f50b1d5f 100644
--- a/src/api2/types/tape/drive.rs
+++ b/src/api2/types/tape/drive.rs
@@ -28,7 +28,7 @@ pub const LTO_DRIVE_PATH_SCHEMA: Schema = StringSchema::new(
 pub const CHANGER_DRIVENUM_SCHEMA: Schema = IntegerSchema::new(
     "Associated changer drive number (requires option changer)")
     .minimum(0)
-    .maximum(8)
+    .maximum(255)
     .default(0)
     .schema();
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 2/6] tape: changer: sg_pt: add SCSI_VOLUME_TAG_LEN const

[Dominik Csapak]

so that we do have less 'magic' constants without description

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no change in v2
 src/tape/changer/sg_pt_changer.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index f4154ced..908311b0 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -33,6 +33,7 @@ use crate::{
 };
 
 const SCSI_CHANGER_DEFAULT_TIMEOUT: usize = 60*5; // 5 minutes
+const SCSI_VOLUME_TAG_LEN: usize = 36;
 
 /// Initialize element status (Inventory)
 pub fn initialize_element_status<F: AsRawFd>(file: &mut F) -> Result<(), Error> {
@@ -509,7 +510,7 @@ impl SubHeader {
     ) -> Result<Option<String>, Error> {
 
         if (self.flags & 128) != 0 { // has PVolTag
-            let tmp = reader.read_exact_allocated(36)?;
+            let tmp = reader.read_exact_allocated(SCSI_VOLUME_TAG_LEN)?;
             if full {
                 let volume_tag = scsi_ascii_to_string(&tmp);
                 return Ok(Some(volume_tag));
@@ -526,7 +527,7 @@ impl SubHeader {
     ) -> Result<Option<String>, Error> {
 
         if (self.flags & 64) != 0 { // has AVolTag
-            let _tmp = reader.read_exact_allocated(36)?;
+            let _tmp = reader.read_exact_allocated(SCSI_VOLUME_TAG_LEN)?;
         }
 
         Ok(None)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 3/6] tape: changer: sg_pt: fix typo

[Dominik Csapak]

ok, since its a private struct

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
no change in v2
 src/tape/changer/sg_pt_changer.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 908311b0..a15d8192 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -536,7 +536,7 @@ impl SubHeader {
 
 #[repr(C, packed)]
 #[derive(Endian)]
-struct TrasnsportDescriptor { // Robot/Griper
+struct TransportDescriptor { // Robot/Griper
     element_address: u16,
     flags1: u8,
     reserved_3: u8,
@@ -665,7 +665,7 @@ fn decode_element_status_page(
 
                 match subhead.element_type_code {
                     1 => {
-                        let desc: TrasnsportDescriptor = unsafe { reader.read_be_value()? };
+                        let desc: TransportDescriptor = unsafe { reader.read_be_value()? };
 
                         let full = (desc.flags1 & 1) != 0;
                         let volume_tag = subhead.parse_optional_volume_tag(&mut reader, full)?;
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 4/6] tape: changer: sg_pt: extend volume tag parsing

[Dominik Csapak]

by also returning the size we read from the reader (we need that later)
and while were at it, also return the alternate volume tag if we have
it (we already read it, so we can return it)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v2
 src/tape/changer/sg_pt_changer.rs | 32 +++++++++++++++++--------------
 1 file changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index a15d8192..9938dfe1 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -507,30 +507,34 @@ impl SubHeader {
         &self,
         reader: &mut R,
         full: bool,
-    ) -> Result<Option<String>, Error> {
+    ) -> Result<(Option<String>, usize), Error> {
 
         if (self.flags & 128) != 0 { // has PVolTag
             let tmp = reader.read_exact_allocated(SCSI_VOLUME_TAG_LEN)?;
             if full {
                 let volume_tag = scsi_ascii_to_string(&tmp);
-                return Ok(Some(volume_tag));
+                return Ok((Some(volume_tag), SCSI_VOLUME_TAG_LEN));
+            } else {
+                return Ok((None, SCSI_VOLUME_TAG_LEN));
             }
         }
-        Ok(None)
+        Ok((None, 0))
     }
 
     // AFAIK, tape changer do not use AlternateVolumeTag
     // but parse anyways, just to be sure
-    fn skip_alternate_volume_tag<R: Read>(
+    fn parse_alternate_volume_tag<R: Read>(
         &self,
         reader: &mut R,
-    ) -> Result<Option<String>, Error> {
+    ) -> Result<(Option<String>, usize), Error> {
 
         if (self.flags & 64) != 0 { // has AVolTag
-            let _tmp = reader.read_exact_allocated(SCSI_VOLUME_TAG_LEN)?;
+            let tmp = reader.read_exact_allocated(SCSI_VOLUME_TAG_LEN)?;
+            let avoltag = scsi_ascii_to_string(&tmp);
+            Ok((Some(avoltag), SCSI_VOLUME_TAG_LEN))
+        } else {
+            Ok((None, 0))
         }
-
-        Ok(None)
     }
 }
 
@@ -668,9 +672,9 @@ fn decode_element_status_page(
                         let desc: TransportDescriptor = unsafe { reader.read_be_value()? };
 
                         let full = (desc.flags1 & 1) != 0;
-                        let volume_tag = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
 
-                        subhead.skip_alternate_volume_tag(&mut reader)?;
+                        subhead.parse_alternate_volume_tag(&mut reader)?;
 
                         let mut reserved = [0u8; 4];
                         reader.read_exact(&mut reserved)?;
@@ -687,9 +691,9 @@ fn decode_element_status_page(
                         let desc: StorageDescriptor = unsafe { reader.read_be_value()? };
 
                         let full = (desc.flags1 & 1) != 0;
-                        let volume_tag = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
 
-                        subhead.skip_alternate_volume_tag(&mut reader)?;
+                        subhead.parse_alternate_volume_tag(&mut reader)?;
 
                         let mut reserved = [0u8; 4];
                         reader.read_exact(&mut reserved)?;
@@ -722,9 +726,9 @@ fn decode_element_status_page(
                         };
 
                         let full = (desc.flags1 & 1) != 0;
-                        let volume_tag = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
 
-                        subhead.skip_alternate_volume_tag(&mut reader)?;
+                        subhead.parse_alternate_volume_tag(&mut reader)?;
 
                         let dvcid: DvcidHead = unsafe { reader.read_be_value()? };
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 5/6] tape: changer: sg_pt: 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.

now we count the data we consume from it, and do not error out if some
fields at the end are missing (we do not need most of them anyway)

also omit the reading of not necessary fields at thend of the descriptor,
since we read the whole remaining descriptor to get the correct
position anyway

this also makes the code to read the rest of the page a bit easier,
since we already counted how much should be left

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
changes from v1:
* correctly subtract the volume tag size when it was read, not only
  when the volume tag had content (empty slots!)
* correctly reduce 'remaining_size' when reading the dvcid fields
* drop the reading of the 'reserved' field at the end, we read
  the remaining descriptor anyway
 src/tape/changer/sg_pt_changer.rs | 93 +++++++++++++++++--------------
 1 file changed, 52 insertions(+), 41 deletions(-)

diff --git a/src/tape/changer/sg_pt_changer.rs b/src/tape/changer/sg_pt_changer.rs
index 9938dfe1..c8510aff 100644
--- a/src/tape/changer/sg_pt_changer.rs
+++ b/src/tape/changer/sg_pt_changer.rs
@@ -661,23 +661,24 @@ fn decode_element_status_page(
                 if reader.is_empty() {
                     break;
                 }
-                if reader.len() < (subhead.descriptor_length as usize) {
+                let mut remaining_size = subhead.descriptor_length as usize;
+                if reader.len() < remaining_size {
                     break;
                 }
 
-                let len_before = reader.len();
-
                 match subhead.element_type_code {
                     1 => {
                         let desc: TransportDescriptor = unsafe { reader.read_be_value()? };
+                        remaining_size -= std::mem::size_of::<TransportDescriptor>();
 
                         let full = (desc.flags1 & 1) != 0;
-                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
-
-                        subhead.parse_alternate_volume_tag(&mut reader)?;
+                        let (volume_tag, read) = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        remaining_size -= read;
 
-                        let mut reserved = [0u8; 4];
-                        reader.read_exact(&mut reserved)?;
+                        if remaining_size >= SCSI_VOLUME_TAG_LEN {
+                            let (_, read) = subhead.parse_alternate_volume_tag(&mut reader)?;
+                            remaining_size -= read;
+                        }
 
                         result.last_element_address = Some(desc.element_address);
 
@@ -689,14 +690,16 @@ fn decode_element_status_page(
                     }
                     2 | 3 => {
                         let desc: StorageDescriptor = unsafe { reader.read_be_value()? };
+                        remaining_size -= std::mem::size_of::<StorageDescriptor>();
 
                         let full = (desc.flags1 & 1) != 0;
-                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        let (volume_tag, read) = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        remaining_size -= read;
 
-                        subhead.parse_alternate_volume_tag(&mut reader)?;
-
-                        let mut reserved = [0u8; 4];
-                        reader.read_exact(&mut reserved)?;
+                        if remaining_size >= SCSI_VOLUME_TAG_LEN {
+                            let (_, read) = subhead.parse_alternate_volume_tag(&mut reader)?;
+                            remaining_size -= read;
+                        }
 
                         result.last_element_address = Some(desc.element_address);
 
@@ -718,6 +721,7 @@ fn decode_element_status_page(
                     }
                     4 => {
                         let desc: TransferDescriptor = unsafe { reader.read_be_value()? };
+                        remaining_size -= std::mem::size_of::<TransferDescriptor>();
 
                         let loaded_slot = if (desc.flags2 & 128) != 0 { // SValid
                             Some(desc.source_storage_element_address as u64)
@@ -726,31 +730,42 @@ fn decode_element_status_page(
                         };
 
                         let full = (desc.flags1 & 1) != 0;
-                        let (volume_tag, _) = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        let (volume_tag, read) = subhead.parse_optional_volume_tag(&mut reader, full)?;
+                        remaining_size -= read;
 
-                        subhead.parse_alternate_volume_tag(&mut reader)?;
+                        if remaining_size >= SCSI_VOLUME_TAG_LEN {
+                            let (_, read) = subhead.parse_alternate_volume_tag(&mut reader)?;
+                            remaining_size -= read;
+                        }
 
-                        let dvcid: DvcidHead = unsafe { reader.read_be_value()? };
+                        let (drive_serial_number, vendor, model) = if remaining_size >= std::mem::size_of::<DvcidHead>() {
+                            let dvcid: DvcidHead = unsafe { reader.read_be_value()? };
+                            remaining_size -= std::mem::size_of::<DvcidHead>();
 
-                        let (drive_serial_number, vendor, model) = match (dvcid.code_set, dvcid.identifier_type) {
-                            (2, 0) => { // Serial number only (Quantum Superloader3 uses this)
-                                let serial = reader.read_exact_allocated(dvcid.identifier_len as usize)?;
-                                let serial = scsi_ascii_to_string(&serial);
-                                (Some(serial), None, None)
-                            }
-                            (2, 1) => {
-                                if dvcid.identifier_len != 34 {
-                                    bail!("got wrong DVCID length");
+                            match (dvcid.code_set, dvcid.identifier_type) {
+                                (2, 0) => { // Serial number only (Quantum Superloader3 uses this)
+                                    let serial = reader.read_exact_allocated(dvcid.identifier_len as usize)?;
+                                    remaining_size -= dvcid.identifier_len as usize;
+                                    let serial = scsi_ascii_to_string(&serial);
+                                    (Some(serial), None, None)
                                 }
-                                let vendor = reader.read_exact_allocated(8)?;
-                                let vendor = scsi_ascii_to_string(&vendor);
-                                let model = reader.read_exact_allocated(16)?;
-                                let model = scsi_ascii_to_string(&model);
-                                let serial = reader.read_exact_allocated(10)?;
-                                let serial = scsi_ascii_to_string(&serial);
-                                (Some(serial), Some(vendor), Some(model))
+                                (2, 1) => {
+                                    if dvcid.identifier_len != 34 {
+                                        bail!("got wrong DVCID length");
+                                    }
+                                    let vendor = reader.read_exact_allocated(8)?;
+                                    let vendor = scsi_ascii_to_string(&vendor);
+                                    let model = reader.read_exact_allocated(16)?;
+                                    let model = scsi_ascii_to_string(&model);
+                                    let serial = reader.read_exact_allocated(10)?;
+                                    let serial = scsi_ascii_to_string(&serial);
+                                    remaining_size -= 8 + 16 + 10;
+                                    (Some(serial), Some(vendor), Some(model))
+                                }
+                                _ => (None, None, None),
                             }
-                            _ => (None, None, None),
+                        } else {
+                            (None, None, None)
                         };
 
                         result.last_element_address = Some(desc.element_address);
@@ -770,15 +785,11 @@ fn decode_element_status_page(
 
                 // 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?
+                if remaining_size > 0 {
+                    if remaining_size > reader.len() {
+                        remaining_size = reader.len();  // reader has not enough data?
                     }
-                    let _ = reader.read_exact_allocated(left_to_read)?;
+                    let _ = reader.read_exact_allocated(remaining_size)?;
                 }
             }
         }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 6/6] tape: changer: sg_pt: improve error message on wrong counts

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
new in v2
 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 c8510aff..c6609b84 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

Re: [pbs-devel] [PATCH proxmox-backup v2 1/6] api: types: CHANGER_DRIVENUM_SCHEMA: increase maximum drives per changer

[Dominik Csapak]

sorry i did not see that you already applied the first three patches, 
just disregard them here, they did not change