From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 13E59775A2 for ; Fri, 22 Oct 2021 09:37:04 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F3C7626F21 for ; Fri, 22 Oct 2021 09:36:33 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 69AF226F16 for ; Fri, 22 Oct 2021 09:36:33 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 361C045F2B; Fri, 22 Oct 2021 09:36:33 +0200 (CEST) Message-ID: Date: Fri, 22 Oct 2021 09:36:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101 Thunderbird/92.0 Content-Language: en-GB To: Proxmox VE development discussion , =?UTF-8?Q?Phillipp_R=c3=b6ll?= References: <20211021084650.26753-1-pr@lima-city.de> From: Thomas Lamprecht In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.056 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -1.742 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH] Check only first part of script parameter for executability X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Oct 2021 07:37:04 -0000 Hi Phillipp, On 21/10/2021 20:15, Phillipp Röll wrote: > I'll skip the legal stuff for now, until we figure out if the patch has a chance > to be merged, ok? sounds fine to me. > >> That would break existing configuration that use a script with whitespace in its >> path name. > > There could be two checks, check if either the full script name is executable OR > just the first part (split by whitespaces). Hmm, that could be an option, for splitting I'd recommend using the `split_args` helper from the PVE::Tools module, it's made for this case and should handle some subtleties better than a plain split on any whitespace. > >> Can you please provide your usecase where the arguments Proxmox VE sets for the >> script aren't enough? So that we've a better picture about possible options. > > We have a backup & restore management system, where the state, retention period etc. > of a backup is stored. All proxmox backup states are updated by a hook script calling > a REST API. We need to pass the ID of the backup from the management system to the > hook script, which is no longer possible since the check was introduced. Is the ID related to the VMID? As in, could it be possible to save a mapping from VMID to current backup ID somewhere (node local or cluster shared on /etc/pve through pmxcfs), e.g., as JSON file, read that file in the hook script and get the BACKUP ID then from that mapping using the VMID as key? Could be slightly more robust and could potentially reduce the amount of REST calls and pmxcfs updates, I mean those resources are not _that_ scarce or performance critical, but with many guests and big clusters the pmxcfs load may get noticeable. But, that said, I'm open into introducing your implementation, that formerly rather worked by accident, as actual and documented possible behaviour. > > We comment/patch these checks out automatically on all systems, but that's a bit > annoying. > > So actually the (much) earlier introduction of the check already broke our use case, > but before today the pain to report it/change something simply wasn't great enough. > It could help leading with that in the commit message or the like, as having an existing use case broken should get you more attention in your favour compared to people thinking you want add a new behaviour as we do not want to break existing use cases if possible, but that side effect was not recognized as being used :)