On Tue, 23 Jul 2024 at 15:16, Paolo Bonzini <[email protected]> wrote:
>
> From: Anthony Harivel <[email protected]>
>
> Introduce a privileged helper to access RAPL MSR.
Hi; Coverity points out an issue with this commit
(CID 1558555):
> +static void coroutine_fn vh_co_entry(void *opaque)
> +{
> + VMSRHelperClient *client = opaque;
> + Error *local_err = NULL;
> + unsigned int peer_pid;
> + uint32_t request[3];
> + uint64_t vmsr;
> + int r;
> +
> + qio_channel_set_blocking(QIO_CHANNEL(client->ioc),
> + false, NULL);
> +
> + qio_channel_set_follow_coroutine_ctx(QIO_CHANNEL(client->ioc), true);
> +
> + /*
> + * Check peer credentials
> + */
> + r = qio_channel_get_peerpid(QIO_CHANNEL(client->ioc),
> + &peer_pid,
> + &local_err);
> + if (r < 0) {
> + error_report_err(local_err);
> + goto out;
Here we have a check for r < 0 that forces an early exit...
> + }
> +
> + while (r < 0) {
...but then immediately we do a while (r < 0). r cannot be < 0
here because we just checked that, so this while loop will
never execute and the whole loop body is dead code.
What was the intention here ?
> + /*
> + * Read the requested MSR
> + * Only RAPL MSR in rapl-msr-index.h is allowed
> + */
> + r = qio_channel_read_all(QIO_CHANNEL(client->ioc),
> + (char *) &request, sizeof(request),
> &local_err);
> + if (r < 0) {
> + error_report_err(local_err);
> + break;
> + }
> +
> + if (!is_msr_allowed(request[0])) {
> + error_report("Requested unallowed msr: %d", request[0]);
> + break;
> + }
> +
> + vmsr = vmsr_read_msr(request[0], request[1]);
> +
> + if (!is_tid_present(peer_pid, request[2])) {
> + error_report("Requested TID not in peer PID: %d %d",
> + peer_pid, request[2]);
> + vmsr = 0;
> + }
> +
> + r = qio_channel_write_all(QIO_CHANNEL(client->ioc),
> + (char *) &vmsr,
> + sizeof(vmsr),
> + &local_err);
> + if (r < 0) {
> + error_report_err(local_err);
> + break;
> + }
> + }
> +out:
> + object_unref(OBJECT(client->ioc));
> + g_free(client);
> +}
thanks
-- PMM
