[{"id":34086,"web_url":"https://patchwork.libcamera.org/comment/34086/","msgid":"<174608905955.299497.16915659723518960903@ping.linuxembedded.co.uk>","date":"2025-05-01T08:44:19","subject":"Re: [PATCH v1 1/2] apps: cam: capture_script: Disallow arrays of\n\tstrings","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-04-22 13:47:50)\n> The current `ControlValue` mechanism does not support arrays\n> of strings, the assignment in the removed snippet will in fact\n> trigger an assertion failure in `ControlValue::set()` because\n> `sizeof(std::string) != ControlValueSize[ControlTypeString]`.\n> \n> Fixes: b35f04b3c194 (\"cam: capture_script: Support parsing array controls\")\n> Signed-off-by: Barnabás Pőcze \n\nI like a clear failure - rather than a guaranteed runtime assertion!\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/apps/cam/capture_script.cpp | 4 ----\n> 1 file changed, 4 deletions(-)\n> \n> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n> index e7e69960e..fdf82efc0 100644\n> --- a/src/apps/cam/capture_script.cpp\n> +++ b/src/apps/cam/capture_script.cpp\n> @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id,\n> value = Span(values.data(), values.size());\n> break;\n> }\n> - case ControlTypeString: {\n> - value = Span(repr.data(), repr.size());\n> - break;\n> - }\n\n\nIs it worth keeping \n\tcase ControlTypeString:\n\nhere as a fall through with a comment explaining that it can't currently\nbe supported ? (I'm fine with a full removal too - just curious if it's\nhelpful to document explicitly that it is 'unsupported' rather than\n'missed' if someone tried to add it..\n\n> default:\n> std::cerr << \"Unsupported control type\" << std::endl;\n> break;\n> -- \n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5560DBE08B\n\tfor ;\n\tThu, 1 May 2025 08:44:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E1E068ACC;\n\tThu, 1 May 2025 10:44:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91ADE68ACC\n\tfor ;\n\tThu, 1 May 2025 10:44:22 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 719D6741;\n\tThu, 1 May 2025 10:44:15 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vaNvktB6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746089055;\n\tbh=aSYLAxW2MLOYoDZ83T95amJ+4HVq1jbUMr35fXhDlIg=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=vaNvktB6Xs21ZivwaPF2S+71dZckk1fr5iRqKOrcKy97MizUps+0xeSiM80nho1m/\n\tStr2I5+l8eOF0nsUWHNR6eZbjfSfgf7GqfLClxKBw8C6aaaM0nAPg1YAmo4bArpp9c\n\tPgBhJz8BvxXMI/UrQ+J6Eisjc7Ky3DgFoOrqRrMg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250422124751.30409-2-barnabas.pocze@ideasonboard.com>","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-2-barnabas.pocze@ideasonboard.com>","Subject":"Re: [PATCH v1 1/2] apps: cam: capture_script: Disallow arrays of\n\tstrings","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 01 May 2025 09:44:19 +0100","Message-ID":"<174608905955.299497.16915659723518960903@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34087,"web_url":"https://patchwork.libcamera.org/comment/34087/","msgid":"<7f38f14c-8472-43a8-ab94-369c89e99109@ideasonboard.com>","date":"2025-05-01T09:15:33","subject":"Re: [PATCH v1 1/2] apps: cam: capture_script: Disallow arrays of\n\tstrings","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 05. 01. 10:44 keltezéssel, Kieran Bingham írta:\n> Quoting Barnabás Pőcze (2025-04-22 13:47:50)\n>> The current `ControlValue` mechanism does not support arrays\n>> of strings, the assignment in the removed snippet will in fact\n>> trigger an assertion failure in `ControlValue::set()` because\n>> `sizeof(std::string) != ControlValueSize[ControlTypeString]`.\n>>\n>> Fixes: b35f04b3c194 (\"cam: capture_script: Support parsing array controls\")\n>> Signed-off-by: Barnabás Pőcze \n> \n> I like a clear failure - rather than a guaranteed runtime assertion!\n> \n> Reviewed-by: Kieran Bingham \n> \n>> ---\n>> src/apps/cam/capture_script.cpp | 4 ----\n>> 1 file changed, 4 deletions(-)\n>>\n>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp\n>> index e7e69960e..fdf82efc0 100644\n>> --- a/src/apps/cam/capture_script.cpp\n>> +++ b/src/apps/cam/capture_script.cpp\n>> @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id,\n>> value = Span(values.data(), values.size());\n>> break;\n>> }\n>> - case ControlTypeString: {\n>> - value = Span(repr.data(), repr.size());\n>> - break;\n>> - }\n> \n> \n> Is it worth keeping\n> \tcase ControlTypeString:\n> \n> here as a fall through with a comment explaining that it can't currently\n> be supported ? (I'm fine with a full removal too - just curious if it's\n> helpful to document explicitly that it is 'unsupported' rather than\n> 'missed' if someone tried to add it..\n\nI'm hoping to make that a compiler error, so I think it shouldn't be an issue.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> default:\n>> std::cerr << \"Unsupported control type\" << std::endl;\n>> break;\n>> -- \n>> 2.49.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E12E5C327D\n\tfor ;\n\tThu, 1 May 2025 09:15:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4709168ADA;\n\tThu, 1 May 2025 11:15:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4037A68ACC\n\tfor ;\n\tThu, 1 May 2025 11:15:38 +0200 (CEST)","from [192.168.33.21] (185.221.143.50.nat.pool.zt.hu\n\t[185.221.143.50])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C04F12E0;\n\tThu, 1 May 2025 11:15:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dNb0Z0dP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746090931;\n\tbh=5SuXJVc2QFKjB6hU9faE+DyEPHeGK/yG+MNDYVm0Ir0=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=dNb0Z0dPBzNITqybHrmiD7qvxoHoZAruSqWUXU1tC6TU9mP0ynHgms5a6WA2wryMP\n\tulknM+0iCAvjJtFXRfjKPxR2iurhN/24QSgR+n7RtMCqrWr1pwyQ8IyYCzpl5/m1L3\n\tUCh0IC8Z1Cyig0jCvpP44KmbeX/HOcyPzGNHWgBM=","Message-ID":"<7f38f14c-8472-43a8-ab94-369c89e99109@ideasonboard.com>","Date":"Thu, 1 May 2025 11:15:33 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 1/2] apps: cam: capture_script: Disallow arrays of\n\tstrings","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20250422124751.30409-1-barnabas.pocze@ideasonboard.com>\n\t<20250422124751.30409-2-barnabas.pocze@ideasonboard.com>\n\t<174608905955.299497.16915659723518960903@ping.linuxembedded.co.uk>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<174608905955.299497.16915659723518960903@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]