* [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs
@ 2023-08-21 7:40 Gabriel Goller
2023-09-07 16:53 ` Thomas Lamprecht
0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Goller @ 2023-08-21 7:40 UTC (permalink / raw)
To: pbs-devel
This will show the ip-address of the client creating
the backup in the logs. For example it will output:
"starting new backup on datastore 'test1' from ::ffff:192.168.1.192:
"host/test/2023-08-21T07:28:10Z"".
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
src/api2/backup/mod.rs | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 652d5baa..0c996e62 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -214,10 +214,21 @@ fn upgrade_to_backup_protocol(
env.debug = debug;
env.last_backup = last_backup;
- env.log(format!(
- "starting new {} on datastore '{}': {:?}",
- worker_type, store, path
- ));
+ match rpcenv.get_client_ip() {
+ Some(address) => {
+ env.log(format!(
+ "starting new {} on datastore '{}' from {}: {:?}",
+ worker_type, store, address.ip(), path
+ ));
+ },
+ None => {
+ env.log(format!(
+ "starting new {} on datastore '{}': {:?}",
+ worker_type, store, path
+ ));
+
+ }
+ }
let service =
H2Service::new(env.clone(), worker.clone(), &BACKUP_API_ROUTER, debug);
--
2.39.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs
2023-08-21 7:40 [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs Gabriel Goller
@ 2023-09-07 16:53 ` Thomas Lamprecht
2023-09-08 14:01 ` Gabriel Goller
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2023-09-07 16:53 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Gabriel Goller
On 21/08/2023 09:40, Gabriel Goller wrote:
> This will show the ip-address of the client creating
> the backup in the logs. For example it will output:
> "starting new backup on datastore 'test1' from ::ffff:192.168.1.192:
> "host/test/2023-08-21T07:28:10Z"".
>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> src/api2/backup/mod.rs | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 652d5baa..0c996e62 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -214,10 +214,21 @@ fn upgrade_to_backup_protocol(
> env.debug = debug;
> env.last_backup = last_backup;
>
> - env.log(format!(
> - "starting new {} on datastore '{}': {:?}",
> - worker_type, store, path
> - ));
> + match rpcenv.get_client_ip() {
could do:
if let Some(ip) = rpcenv.get_client_ip().map(|addr| addr.ip()) {
This all feels a bit bloated for what it does, and even if it seems like nitpicking,
such things can add up and make maintainability harder in the long run.
You could get the actual IP via a .map and work on that, i.e., I'd to something like
either:
let msg = if let Some(ip) = rpcenv.get_client_ip().map(|addr| addr.ip()) {
format!("starting new {worker_type} on datastore '{store}' from {ip}: {path:?}")
} else {
format!("starting new {worker_type} on datastore '{store}': {path:?}")
};
env.log(msg);
or:
let origin = match rpcenv.get_client_ip().map(|addr| addr.ip()) {
Some(ip) => format!(" from {ip}"),
None => "".into(),
};
env.log(format!("starting new {worker_type} on datastore '{store}'{origin}: {path:?}"));
Where the latter reads slightly nicer IMO but has two allocation, maybe you got a better
idea.
Oh, and for quite a few users the IPv4 mapped as IPv6 notation looks odd, so dropping the
::ffff: prefix might be worth it, but no hard feelings, we can always change that if there
are complaints..
> + Some(address) => {
> + env.log(format!(
> + "starting new {} on datastore '{}' from {}: {:?}",
> + worker_type, store, address.ip(), path
> + ));
> + },
> + None => {
> + env.log(format!(
> + "starting new {} on datastore '{}': {:?}",
> + worker_type, store, path
> + ));
> +
extra empty line here..
> + }
> + }
>
> let service =
> H2Service::new(env.clone(), worker.clone(), &BACKUP_API_ROUTER, debug);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs
2023-09-07 16:53 ` Thomas Lamprecht
@ 2023-09-08 14:01 ` Gabriel Goller
2023-09-13 7:08 ` Thomas Lamprecht
0 siblings, 1 reply; 5+ messages in thread
From: Gabriel Goller @ 2023-09-08 14:01 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On 9/7/23 18:53, Thomas Lamprecht wrote:
> On 21/08/2023 09:40, Gabriel Goller wrote:
>> This will show the ip-address of the client creating
>> the backup in the logs. For example it will output:
>> "starting new backup on datastore 'test1' from ::ffff:192.168.1.192:
>> "host/test/2023-08-21T07:28:10Z"".
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>> src/api2/backup/mod.rs | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
>> index 652d5baa..0c996e62 100644
>> --- a/src/api2/backup/mod.rs
>> +++ b/src/api2/backup/mod.rs
>> @@ -214,10 +214,21 @@ fn upgrade_to_backup_protocol(
>> env.debug = debug;
>> env.last_backup = last_backup;
>>
>> - env.log(format!(
>> - "starting new {} on datastore '{}': {:?}",
>> - worker_type, store, path
>> - ));
>> + match rpcenv.get_client_ip() {
>
> could do:
>
> if let Some(ip) = rpcenv.get_client_ip().map(|addr| addr.ip()) {
>
> This all feels a bit bloated for what it does, and even if it seems like nitpicking,
> such things can add up and make maintainability harder in the long run.
>
> You could get the actual IP via a .map and work on that, i.e., I'd to something like
> either:
>
> let msg = if let Some(ip) = rpcenv.get_client_ip().map(|addr| addr.ip()) {
> format!("starting new {worker_type} on datastore '{store}' from {ip}: {path:?}")
> } else {
> format!("starting new {worker_type} on datastore '{store}': {path:?}")
> };
> env.log(msg);
>
>
> or:
>
> let origin = match rpcenv.get_client_ip().map(|addr| addr.ip()) {
> Some(ip) => format!(" from {ip}"),
> None => "".into(),
> };
> env.log(format!("starting new {worker_type} on datastore '{store}'{origin}: {path:?}"));
>
>
> Where the latter reads slightly nicer IMO but has two allocation, maybe you got a better
> idea.
Second one looks better I think.
> Oh, and for quite a few users the IPv4 mapped as IPv6 notation looks odd, so dropping the
> ::ffff: prefix might be worth it, but no hard feelings, we can always change that if there
> are complaints..
Noticed that too, but IMO staying 'correct' is more important than
making the output pretty.
But we can always change it later...
>> + Some(address) => {
>> + env.log(format!(
>> + "starting new {} on datastore '{}' from {}: {:?}",
>> + worker_type, store, address.ip(), path
>> + ));
>> + },
>> + None => {
>> + env.log(format!(
>> + "starting new {} on datastore '{}': {:?}",
>> + worker_type, store, path
>> + ));
>> +
> extra empty line here..
>> + }
>> + }
>>
>> let service =
>> H2Service::new(env.clone(), worker.clone(), &BACKUP_API_ROUTER, debug);
>>
Submitted a new patch.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs
2023-09-08 14:01 ` Gabriel Goller
@ 2023-09-13 7:08 ` Thomas Lamprecht
2023-09-13 8:12 ` Gabriel Goller
0 siblings, 1 reply; 5+ messages in thread
From: Thomas Lamprecht @ 2023-09-13 7:08 UTC (permalink / raw)
To: Gabriel Goller, Proxmox Backup Server development discussion
Am 08/09/2023 um 16:01 schrieb Gabriel Goller:
> On 9/7/23 18:53, Thomas Lamprecht wrote:
>> Oh, and for quite a few users the IPv4 mapped as IPv6 notation looks odd, so dropping the
>> ::ffff: prefix might be worth it, but no hard feelings, we can always change that if there
>> are complaints..
> Noticed that too, but IMO staying 'correct' is more important than making the output pretty.
How is a IPv4 mapped as IPv6 more correct than a IPv4, it's literally the same thing
in two different notations?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs
2023-09-13 7:08 ` Thomas Lamprecht
@ 2023-09-13 8:12 ` Gabriel Goller
0 siblings, 0 replies; 5+ messages in thread
From: Gabriel Goller @ 2023-09-13 8:12 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On 9/13/23 09:08, Thomas Lamprecht wrote:
> Am 08/09/2023 um 16:01 schrieb Gabriel Goller:
>> On 9/7/23 18:53, Thomas Lamprecht wrote:
>>> Oh, and for quite a few users the IPv4 mapped as IPv6 notation looks odd, so dropping the
>>> ::ffff: prefix might be worth it, but no hard feelings, we can always change that if there
>>> are complaints..
>> Noticed that too, but IMO staying 'correct' is more important than making the output pretty.
> How is a IPv4 mapped as IPv6 more correct than a IPv4, it's literally the same thing
> in two different notations?
You're right, they are the same thing, but I wanted to maintain
consistency with
how we currently get the ip address and don't make any unnecessary string
operations.
But, as I said, if you feel like this should be stripped, I'll go ahead
and change it.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-09-13 8:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-21 7:40 [pbs-devel] [PATCH proxmox-backup] close #3777: api: Add source information to backup logs Gabriel Goller
2023-09-07 16:53 ` Thomas Lamprecht
2023-09-08 14:01 ` Gabriel Goller
2023-09-13 7:08 ` Thomas Lamprecht
2023-09-13 8:12 ` Gabriel Goller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox