all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Hanreich <s.hanreich@proxmox.com>
To: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: pbs-devel@lists.proxmox.com
Subject: Re: [pbs-devel] [PATCH proxmox-backup] fix #4301: correctly pass rate limit parameters to API
Date: Thu, 20 Oct 2022 16:11:46 +0200	[thread overview]
Message-ID: <40c5146e-21eb-d781-bc2f-0cac60a522c4@proxmox.com> (raw)
In-Reply-To: <20221020140602.sxzfyzajry25mqxq@casey.proxmox.com>



On 10/20/22 16:06, Wolfgang Bumiller wrote:
> On Thu, Oct 20, 2022 at 03:50:59PM +0200, Stefan Hanreich wrote:
>>
>>
>> On 10/20/22 15:44, Wolfgang Bumiller wrote:
>>> On Thu, Oct 20, 2022 at 02:59:46PM +0200, Stefan Hanreich wrote:
>>>>
>>>>
>>>> On 10/20/22 14:48, Wolfgang Bumiller wrote:
>>>>> On Thu, Oct 20, 2022 at 01:37:31PM +0200, Stefan Hanreich wrote:
>>>>>> With the old code the rate limit parameters got passed in their own
>>>>>> dictionary under the limit key, but the API expects the rate-limit
>>>>>> settings as top-level keys. This commit correctly sets the rate-limit
>>>>>> parameters so the API actually uses them.
>>>>>>
>>>>>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>>>>>> ---
>>>>>>     src/bin/proxmox-backup-manager.rs | 14 ++++++++++++--
>>>>>>     1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
>>>>>> index 58e7e33a..cdd1037d 100644
>>>>>> --- a/src/bin/proxmox-backup-manager.rs
>>>>>> +++ b/src/bin/proxmox-backup-manager.rs
>>>>>> @@ -2,7 +2,7 @@ use std::collections::HashMap;
>>>>>>     use std::io::{self, Write};
>>>>>>     use std::str::FromStr;
>>>>>> -use anyhow::Error;
>>>>>> +use anyhow::{Error, format_err};
>>>>>>     use serde_json::{json, Value};
>>>>>>     use proxmox_router::{cli::*, RpcEnvironment};
>>>>>> @@ -297,7 +297,6 @@ async fn pull_datastore(
>>>>>>             "store": store,
>>>>>>             "remote": remote,
>>>>>>             "remote-store": remote_store,
>>>>>> -        "limit": limit,
>>>>>>         });
>>>>>>         if remote_ns.is_some() {
>>>>>> @@ -320,6 +319,17 @@ async fn pull_datastore(
>>>>>>             args["remove-vanished"] = Value::from(remove_vanished);
>>>>>>         }
>>>>>> +    let args_map = args
>>>>>> +        .as_object_mut()
>>>>>> +        .ok_or_else(|| format_err!("args is not an Object"))?;
>>>>>
>>>>> ^ We create the `args` map only a few lines further up, so it would be
>>>>> fine to just `.unwrap()` here. And it would be nicer to keep the access
>>>>> short (iow move the `.as_object_mut()` down to where it's used for `.append()`)
>>>>>
>>>>
>>>> Can replace it with unwrap(), shouldn't be a problem.
>>>>
>>>> The reason why I stored it in a variable was that with a subsequent patch I
>>>> will also append another map to the args, but I could then just call
>>>> .as_object_mut() twice, what do you think?
>>>
>>> I suppose that's fine. If you already know such things while sending a
>>> patch without the followups being part of the same series, you can note
>>> such things after the `---` in the patch mail, that way it won't
>>> needlessly end up in the commit message, but still be visible to the
>>> reviewer.
>>>
>>> Come to think of it, there are a lot of `args[key] = value;` assignments
>>> which could probably benefit from already having the `&mut Map` instead
>>> of the `Value` enum.
>>>
>>> Maybe should change it like this
>>>
>>>       -    let mut args = json!(...)
>>>       +    let mut args_value = json(!...);
>>>       +    let args = args_value.as_object_mut().unwrap();
>>>
>>> right where it is created, this way the unwrap will stay even closer to
>>> the creation, the remaining assignments already see the object type, and
>>> you can call append, too.
>>> Finally, the `Some(args)` would need to be changed to `Some(args_value)`
>>> in the `post()` call.
>>>
>>
>> I will keep that in mind for future patches, thanks for the info!
>>
>> The problem is when assigning via args[key] = value on a Map, it actually
>> panics if the key doesn't exist in the map [1], so I'm afraid this isn't a
>> possibility unless I'm missing something.
> 
> Can be changed to use `.insert()` ;-)

yes, but I thought that would just be needlessly verbose since all 
`args[key] = value` would be replaced with some kind of

```
args.insert(
   key.to_string(),
   value.into() / json!(value)
);
```

I can definitely change it to use `insert()` in my subsequent patch if 
you want me to - just say the word ;)




  reply	other threads:[~2022-10-20 14:12 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-20 11:37 Stefan Hanreich
2022-10-20 12:48 ` Wolfgang Bumiller
2022-10-20 12:59   ` Stefan Hanreich
2022-10-20 13:44     ` Wolfgang Bumiller
2022-10-20 13:50       ` Stefan Hanreich
2022-10-20 14:06         ` Wolfgang Bumiller
2022-10-20 14:11           ` Stefan Hanreich [this message]
2022-10-20 14:20 Wolfgang Bumiller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=40c5146e-21eb-d781-bc2f-0cac60a522c4@proxmox.com \
    --to=s.hanreich@proxmox.com \
    --cc=pbs-devel@lists.proxmox.com \
    --cc=w.bumiller@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal