[pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[Christian Ebner]

Instead of returning an Option for the Codename variant, with None for
unknowns, extend the enum by an Unknown variant with additional internal
type to avoid misuse of this variant.

Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-apt/src/repositories/file.rs    | 12 +++--
 proxmox-apt/src/repositories/release.rs | 61 +++++++++++++++----------
 2 files changed, 46 insertions(+), 27 deletions(-)

diff --git a/proxmox-apt/src/repositories/file.rs b/proxmox-apt/src/repositories/file.rs
index b4c6b08..b8a2c7f 100644
--- a/proxmox-apt/src/repositories/file.rs
+++ b/proxmox-apt/src/repositories/file.rs
@@ -405,10 +405,14 @@ impl APTRepositoryFile {
                     add_info("warning", message_old(base_suite));
                 }
 
-                if Some(codename) == current_codename.next() {
-                    add_info("ignore-pre-upgrade-warning", message_new(base_suite));
-                } else if codename > current_codename {
-                    add_info("warning", message_new(base_suite));
+                match current_codename.next() {
+                    name if name == codename => {
+                        add_info("ignore-pre-upgrade-warning", message_new(base_suite));
+                    }
+                    DebianCodename::Unknown(_, _) if codename > current_codename => {
+                        add_info("warning", message_new(base_suite));
+                    }
+                    _ => {}
                 }
 
                 if let Some(require_suffix) = require_suffix {
diff --git a/proxmox-apt/src/repositories/release.rs b/proxmox-apt/src/repositories/release.rs
index da391e5..508a9e0 100644
--- a/proxmox-apt/src/repositories/release.rs
+++ b/proxmox-apt/src/repositories/release.rs
@@ -3,8 +3,17 @@ use std::io::{BufRead, BufReader};
 
 use anyhow::{bail, format_err, Error};
 
+mod private {
+    // public types in private modules are unnamable by external users, this is similar to the
+    // "sealed trait" pattern
+    #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
+    pub struct Internal;
+}
+use private::Internal;
+
 /// The code names of Debian releases. Does not include `sid`.
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+#[repr(u8)]
 pub enum DebianCodename {
     Lenny = 5,
     Squeeze,
@@ -15,13 +24,37 @@ pub enum DebianCodename {
     Bullseye,
     Bookworm,
     Trixie,
+    Unknown(u8, Internal),
 }
 
 impl DebianCodename {
-    pub fn next(&self) -> Option<Self> {
-        match (*self as u8 + 1).try_into() {
-            Ok(codename) => Some(codename),
-            Err(_) => None,
+    pub fn value(&self) -> u8 {
+        match self {
+            Self::Unknown(number, _) => *number,
+            // see 'arbitrary_enum_discriminant' feature for why this is safe:
+            // https://rust-lang.github.io/rfcs/2363-arbitrary-enum-discriminant.html
+            other => unsafe { *(other as *const Self as *const u8) },
+        }
+    }
+
+    pub fn next(&self) -> Self {
+        (self.value() + 1).into()
+    }
+}
+
+impl From<u8> for DebianCodename {
+    fn from(number: u8) -> Self {
+        match number {
+            5 => Self::Lenny,
+            6 => Self::Squeeze,
+            7 => Self::Wheezy,
+            8 => Self::Jessie,
+            9 => Self::Stretch,
+            10 => Self::Buster,
+            11 => Self::Bullseye,
+            12 => Self::Bookworm,
+            13 => Self::Trixie,
+            number => Self::Unknown(number, Internal),
         }
     }
 }
@@ -45,25 +78,6 @@ impl TryFrom<&str> for DebianCodename {
     }
 }
 
-impl TryFrom<u8> for DebianCodename {
-    type Error = Error;
-
-    fn try_from(number: u8) -> Result<Self, Error> {
-        match number {
-            5 => Ok(DebianCodename::Lenny),
-            6 => Ok(DebianCodename::Squeeze),
-            7 => Ok(DebianCodename::Wheezy),
-            8 => Ok(DebianCodename::Jessie),
-            9 => Ok(DebianCodename::Stretch),
-            10 => Ok(DebianCodename::Buster),
-            11 => Ok(DebianCodename::Bullseye),
-            12 => Ok(DebianCodename::Bookworm),
-            13 => Ok(DebianCodename::Trixie),
-            _ => bail!("unknown Debian release number '{}'", number),
-        }
-    }
-}
-
 impl Display for DebianCodename {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
@@ -76,6 +90,7 @@ impl Display for DebianCodename {
             DebianCodename::Bullseye => write!(f, "bullseye"),
             DebianCodename::Bookworm => write!(f, "bookworm"),
             DebianCodename::Trixie => write!(f, "trixie"),
+            DebianCodename::Unknown(number, _) => write!(f, "unknown"),
         }
     }
 }
-- 
2.30.2

[pve-devel] [PATCH proxmox stable-2 2/2] apt: repos: Remove unneeded unwrap for codename variant

[Christian Ebner]

The call to next now does not return an Option, but rahter contains a
dedicated Unknown variant. Therefore the unwrap call can be avoided and
the panic to calls to `pveversion` when /etc/os-release contained
`VERSION_CODENAME=trixie` can be avoided.

Reported in the community forum:
https://forum.proxmox.com/threads/141004/

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-apt/src/repositories/mod.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/proxmox-apt/src/repositories/mod.rs b/proxmox-apt/src/repositories/mod.rs
index 6588b90..ccb49cd 100644
--- a/proxmox-apt/src/repositories/mod.rs
+++ b/proxmox-apt/src/repositories/mod.rs
@@ -110,8 +110,9 @@ pub fn standard_repositories(
                     || repo.is_referenced_repository(
                         entry.handle,
                         product,
-                        &suite.next().unwrap().to_string(),
-                ) {
+                        &suite.next().to_string(),
+                    )
+                {
                     entry.status = Some(repo.enabled);
                 }
             }
-- 
2.30.2

Re: [pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[Fiona Ebner]

Am 05.02.24 um 18:08 schrieb Christian Ebner:
> Instead of returning an Option for the Codename variant, with None for
> unknowns, extend the enum by an Unknown variant with additional internal
> type to avoid misuse of this variant.
> 
> Co-authored-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  proxmox-apt/src/repositories/file.rs    | 12 +++--
>  proxmox-apt/src/repositories/release.rs | 61 +++++++++++++++----------
>  2 files changed, 46 insertions(+), 27 deletions(-)
> 
> diff --git a/proxmox-apt/src/repositories/file.rs b/proxmox-apt/src/repositories/file.rs
> index b4c6b08..b8a2c7f 100644
> --- a/proxmox-apt/src/repositories/file.rs
> +++ b/proxmox-apt/src/repositories/file.rs
> @@ -405,10 +405,14 @@ impl APTRepositoryFile {
>                      add_info("warning", message_old(base_suite));
>                  }
>  
> -                if Some(codename) == current_codename.next() {
> -                    add_info("ignore-pre-upgrade-warning", message_new(base_suite));
> -                } else if codename > current_codename {
> -                    add_info("warning", message_new(base_suite));
> +                match current_codename.next() {
> +                    name if name == codename => {
> +                        add_info("ignore-pre-upgrade-warning", message_new(base_suite));
> +                    }
> +                    DebianCodename::Unknown(_, _) if codename > current_codename => {
> +                        add_info("warning", message_new(base_suite));
> +                    }
> +                    _ => {}

Like this, the warning is lost when we match a known codename that is
newer than the current one. What is the issue with the current code you
are trying to address?

As for the match, you could pull in the case where the suite is older
(the if block just above this code) and rather do an exhaustive match.

Re: [pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[Christian Ebner]

> >  
> > -                if Some(codename) == current_codename.next() {
> > -                    add_info("ignore-pre-upgrade-warning", message_new(base_suite));
> > -                } else if codename > current_codename {
> > -                    add_info("warning", message_new(base_suite));
> > +                match current_codename.next() {
> > +                    name if name == codename => {
> > +                        add_info("ignore-pre-upgrade-warning", message_new(base_suite));
> > +                    }
> > +                    DebianCodename::Unknown(_, _) if codename > current_codename => {
> > +                        add_info("warning", message_new(base_suite));
> > +                    }
> > +                    _ => {}
> 
> Like this, the warning is lost when we match a known codename that is
> newer than the current one. What is the issue with the current code you
> are trying to address?

There is no issue with the current code, I just refactored it to use the newly introduced variant instead of the Option.
Am I missing something? This should behave just like the code before.

> 
> As for the match, you could pull in the case where the suite is older
> (the if block just above this code) and rather do an exhaustive match.

Re: [pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[Fiona Ebner]

Am 06.02.24 um 09:48 schrieb Christian Ebner:
> 
>>>  
>>> -                if Some(codename) == current_codename.next() {
>>> -                    add_info("ignore-pre-upgrade-warning", message_new(base_suite));
>>> -                } else if codename > current_codename {
>>> -                    add_info("warning", message_new(base_suite));
>>> +                match current_codename.next() {
>>> +                    name if name == codename => {
>>> +                        add_info("ignore-pre-upgrade-warning", message_new(base_suite));
>>> +                    }
>>> +                    DebianCodename::Unknown(_, _) if codename > current_codename => {
>>> +                        add_info("warning", message_new(base_suite));
>>> +                    }
>>> +                    _ => {}
>>
>> Like this, the warning is lost when we match a known codename that is
>> newer than the current one. What is the issue with the current code you
>> are trying to address?
> 
> There is no issue with the current code, I just refactored it to use the newly introduced variant instead of the Option.
> Am I missing something? This should behave just like the code before.
> 

You only match the current codename and DebianCodename::Unknown(_, _).
Any known newer codename will just fall through to the default arm (and
the default arm can/should be avoided by pulling in the check for the
older codename from above).

Re: [pve-devel] [PATCH proxmox master stable-2 1/2] apt: repos: extend `Codename` by `Unknown` variant

[Christian Ebner]

> You only match the current codename and DebianCodename::Unknown(_, _).
> Any known newer codename will just fall through to the default arm (and
> the default arm can/should be avoided by pulling in the check for the
> older codename from above).

Ah now I see, thanks for noticing and for clarification, will send a v2 fixing this.