[PATCH v2] pipeline: rpi: Fix potential empty optional read

[Naushir Patuck]

Hi Barnabás,

Thank you for this fix

On Tue, 1 Apr 2025 at 14:11, Barnabás Pőcze
<barnabas.pocze at ideasonboard.com> wrote:
>
> If `!target`, then `*target` is undefined behaviour, so check if the optional
> is empty when printing the error message. Simplify the check as well.
>
> Fixes: 6c71ee1f153051 ("pipeline: raspberrypi: Introduce PipelineHandlerBase class")
> Fixes: 841ef2b4bb08ba ("pipeline: rpi: Add support for Raspberry Pi 5")
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes in v2:
>   * fix the same issue in pisp as well
> ---
>  src/libcamera/pipeline/rpi/pisp/pisp.cpp | 4 ++--
>  src/libcamera/pipeline/rpi/vc4/vc4.cpp   | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> index 42ca7c80b..91e7f4c94 100644
> --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp
> @@ -1350,9 +1350,9 @@ int PiSPCameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject>
>         }
>
>         std::optional<std::string> target = (*root)["target"].get<std::string>();
> -       if (!target || *target != "pisp") {
> +       if (target != "pisp") {

I'm a bit curious about this change.

Should we keep the if (!target || ...) test?  If "target" key is not
present in the yaml, the optional will be a nullopt, and we then deref
for the string comparison.  Does this work out correctly by compiler
magic, or is it undefined behavior?  Ditto for the change below.

Regards,
Naush


>                 LOG(RPI, Error) << "Unexpected target reported: expected \"pisp\", got "
> -                               << *target;
> +                               << (target ? target->c_str() : "(unknown)");
>                 return -EINVAL;
>         }
>
> diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> index fd8d84b14..fe910bdf2 100644
> --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> @@ -510,9 +510,9 @@ int Vc4CameraData::platformPipelineConfigure(const std::unique_ptr<YamlObject> &
>         }
>
>         std::optional<std::string> target = (*root)["target"].get<std::string>();
> -       if (!target || *target != "bcm2835") {
> +       if (target != "bcm2835") {
>                 LOG(RPI, Error) << "Unexpected target reported: expected \"bcm2835\", got "
> -                               << *target;
> +                               << (target ? target->c_str() : "(unknown)");
>                 return -EINVAL;
>         }
>
> --
> 2.49.0