[v1,1/2] apps: cam: capture_script: Disallow arrays of strings

23205 diff mbox series

The current `ControlValue` mechanism does not support arrays
of strings, the assignment in the removed snippet will in fact
trigger an assertion failure in `ControlValue::set()` because
`sizeof(std::string) != ControlValueSize[ControlTypeString]`.

Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls")
Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/apps/cam/capture_script.cpp | 4 ----
 1 file changed, 4 deletions(-)

Quoting Barnabás Pőcze (2025-04-22 13:47:50)
> The current `ControlValue` mechanism does not support arrays
> of strings, the assignment in the removed snippet will in fact
> trigger an assertion failure in `ControlValue::set()` because
> `sizeof(std::string) != ControlValueSize[ControlTypeString]`.
> 
> Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls")
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

I like a clear failure - rather than a guaranteed runtime assertion!

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  src/apps/cam/capture_script.cpp | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
> index e7e69960e..fdf82efc0 100644
> --- a/src/apps/cam/capture_script.cpp
> +++ b/src/apps/cam/capture_script.cpp
> @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id,
>                 value = Span<const float>(values.data(), values.size());
>                 break;
>         }
> -       case ControlTypeString: {
> -               value = Span<const std::string>(repr.data(), repr.size());
> -               break;
> -       }


Is it worth keeping 
	case ControlTypeString:

here as a fall through with a comment explaining that it can't currently
be supported ? (I'm fine with a full removal too - just curious if it's
helpful to document explicitly that it is 'unsupported' rather than
'missed' if someone tried to add it..

>         default:
>                 std::cerr << "Unsupported control type" << std::endl;
>                 break;
> -- 
> 2.49.0
>

Hi


2025. 05. 01. 10:44 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-04-22 13:47:50)
>> The current `ControlValue` mechanism does not support arrays
>> of strings, the assignment in the removed snippet will in fact
>> trigger an assertion failure in `ControlValue::set()` because
>> `sizeof(std::string) != ControlValueSize[ControlTypeString]`.
>>
>> Fixes: b35f04b3c194 ("cam: capture_script: Support parsing array controls")
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> I like a clear failure - rather than a guaranteed runtime assertion!
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> ---
>>   src/apps/cam/capture_script.cpp | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/src/apps/cam/capture_script.cpp b/src/apps/cam/capture_script.cpp
>> index e7e69960e..fdf82efc0 100644
>> --- a/src/apps/cam/capture_script.cpp
>> +++ b/src/apps/cam/capture_script.cpp
>> @@ -578,10 +578,6 @@ ControlValue CaptureScript::parseArrayControl(const ControlId *id,
>>                  value = Span<const float>(values.data(), values.size());
>>                  break;
>>          }
>> -       case ControlTypeString: {
>> -               value = Span<const std::string>(repr.data(), repr.size());
>> -               break;
>> -       }
> 
> 
> Is it worth keeping
> 	case ControlTypeString:
> 
> here as a fall through with a comment explaining that it can't currently
> be supported ? (I'm fine with a full removal too - just curious if it's
> helpful to document explicitly that it is 'unsupported' rather than
> 'missed' if someone tried to add it..

I'm hoping to make that a compiler error, so I think it shouldn't be an issue.


Regards,
Barnabás Pőcze

> 
>>          default:
>>                  std::cerr << "Unsupported control type" << std::endl;
>>                  break;
>> -- 
>> 2.49.0
>>