[pbs-devel] superseded: Re: [PATCH backup v2] http_client: fallback if XDG_RUNTIME_DIR is not set

[Maximiliano Sandoval]

Wolfgang Bumiller <w.bumiller at proxmox.com> writes:

> On Mon, Apr 14, 2025 at 04:22:17PM +0200, Maximiliano Sandoval wrote:
>> xdg::BaseDirectory's [place_directory_file] errors out if
>
> Should be `place_runtime_file`, since other directories will fall back
> to the regular $HOME paths.
>
> For the runtime dir this makes sense. A user who is not logged into a
> seat does not *have* one.
>
>> `XDG_RUNTIME_DIR` is not set.
>> 
>> This is not ideal, as per the the base directory [specification]:
>> 
>> > If $XDG_RUNTIME_DIR is not set applications should fall back to a
>> replacement directory with similar capabilities and print a warning
>> message. Applications should use this directory for communication and
>> synchronization purposes and should not place larger files in it, since
>> it might reside in runtime memory and cannot necessarily be swapped out
>> to disk.
>
> ^ This is not useful information for why this patch is doing what it
> does.
>
>> 
>> At the moment, running the proxmox-backup-client with sudo will print an error:
>> ```
>> storing login ticket failed: $XDG_RUNTIME_DIR must be set
>> ```
>> 
>> We add a helper that places a runtime file `basename` which fallbacks to
>> `/run/user/{uid}/{prefix}/{basename}`.
>
> A runtime directory is created when the user logs into a seat, so a
> logged in user always has one, so if not, is the `/run/user/$uid`
> directory really the right choice? Are there any other tools that do
> this?
>
> For this case I'd argue that we should instead switch from the runtime
> directory to the cache directory. This should also work via sudo and
> makes more sense. The ticket can still be used if the machine reboots,
> after all.
>> 
>> [place_directory_file] https://docs.rs/xdg/latest/xdg/struct.BaseDirectories.html#method.place_runtime_file
>> [specification]: https://specifications.freedesktop.org/basedir-spec/latest/
>> 
>> Signed-off-by: Maximiliano Sandoval <m.sandoval at proxmox.com>
>> ---
>> Differences from v1:
>>  - Actually use `prefix` argument in fallback branch
>>  - Stop checking for root, always use /run/user/{uid}/{prefix}
>> 
>>  pbs-client/src/http_client.rs | 50 +++++++++++++++++++++++++----------
>>  1 file changed, 36 insertions(+), 14 deletions(-)
>> 
>> diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.rs
>> index c95def07b..9b1a9595c 100644
>> --- a/pbs-client/src/http_client.rs
>> +++ b/pbs-client/src/http_client.rs
>> @@ -1,8 +1,9 @@
>>  use std::io::{IsTerminal, Write};
>> +use std::path::PathBuf;
>>  use std::sync::{Arc, Mutex, RwLock};
>>  use std::time::Duration;
>>  
>> -use anyhow::{bail, format_err, Error};
>> +use anyhow::{bail, format_err, Context, Error};
>>  use futures::*;
>>  #[cfg(not(target_feature = "crt-static"))]
>>  use hyper::client::connect::dns::GaiResolver;
>> @@ -27,7 +28,7 @@ use proxmox_async::broadcast_future::BroadcastFuture;
>>  use proxmox_http::client::HttpsConnector;
>>  use proxmox_http::uri::{build_authority, json_object_to_query};
>>  use proxmox_http::{ProxyConfig, RateLimiter};
>> -use proxmox_log::{error, info, warn};
>> +use proxmox_log::{debug, error, info, warn};
>>  
>>  use pbs_api_types::percent_encoding::DEFAULT_ENCODE_SET;
>>  use pbs_api_types::{Authid, RateLimitConfig, Userid};
>> @@ -216,10 +217,7 @@ pub struct HttpClient {
>>  
>>  /// Delete stored ticket data (logout)
>>  pub fn delete_ticket_info(prefix: &str, server: &str, username: &Userid) -> Result<(), Error> {
>> -    let base = BaseDirectories::with_prefix(prefix)?;
>> -
>> -    // usually /run/user/<uid>/...
>> -    let path = base.place_runtime_file("tickets")?;
>> +    let path = place_runtime_file(prefix, "tickets")?;
>>  
>>      let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600);
>>  
>> @@ -305,10 +303,7 @@ fn store_ticket_info(
>>      ticket: &str,
>>      token: &str,
>>  ) -> Result<(), Error> {
>> -    let base = BaseDirectories::with_prefix(prefix)?;
>> -
>> -    // usually /run/user/<uid>/...
>> -    let path = base.place_runtime_file("tickets")?;
>> +    let path = place_runtime_file(prefix, "tickets")?;
>>  
>>      let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600);
>>  
>> @@ -345,10 +340,9 @@ fn store_ticket_info(
>>  }
>>  
>>  fn load_ticket_info(prefix: &str, server: &str, userid: &Userid) -> Option<(String, String)> {
>> -    let base = BaseDirectories::with_prefix(prefix).ok()?;
>> -
>> -    // usually /run/user/<uid>/...
>> -    let path = base.place_runtime_file("tickets").ok()?;
>> +    let path = place_runtime_file(prefix, "tickets")
>> +        .inspect_err(|err| error!("could not place runtime file: {err:#}"))
>> +        .ok()?;
>>      let data = file_get_json(path, None).ok()?;
>>      let now = proxmox_time::epoch_i64();
>>      let ticket_lifetime = proxmox_auth_api::TICKET_LIFETIME - 60;
>> @@ -1181,3 +1175,31 @@ impl H2Client {
>>          Ok(request)
>>      }
>>  }
>> +
>> +// Returns an absolute path in `XDG_RUNTIME_DIR` where a runtime file may be
>> +// stored. Leading directories in the returned path are pre-created; if that is
>> +// not possible, an error is returned.
>> +//
>> +// Similar to [BaseDirectories::place_runtime_file] but will fall back to
>> +// `/run/user/{uid}/{prefix}` if the `XDG_RUNTIME_DIR` variable is not set.
>> +fn place_runtime_file(prefix: &str, basename: &str) -> Result<PathBuf, Error> {
>> +    let base =
>> +        BaseDirectories::with_prefix(prefix).with_context(|| "failed to get base directories")?;
>> +
>> +    let path = if base.has_runtime_directory() {
>> +        base.place_runtime_file(basename)
>> +            .with_context(|| format!("failed to place runtime file {basename}"))?
>> +    } else {
>> +        let uid = nix::unistd::Uid::current();
>> +        let path = PathBuf::from(format!("/run/user/{uid}/{prefix}/"));
>> +        std::fs::create_dir_all(&path)?;
>> +        debug!(
>> +            "XDG_RUNTIME_DIR is not set, using {} as fallback",
>> +            path.display()
>> +        );
>> +        path.join(basename)
>> +    };
>> +    debug!("placing {basename} at {}", path.display());
>> +
>> +    Ok(path)
>> +}
>> -- 
>> 2.39.5


superseded by
https://lore.proxmox.com/pbs-devel/20250416125651.334868-1-m.sandoval@proxmox.com/T/#t.