[PATCH 0/5] tools: address Coverity UNUSED issues

[PATCH 0/5] tools: address Coverity UNUSED issues

[Jan Beulich]

There a number of cases where Coverity has spotted writes to variables
when the written values wouldn't be used subsequently. The patches here
are independent of one another, except for this common theme.

1: xen-mfndump: drop dead assignment to "page" from lookup_pte_func()
2: libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()
3: libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()
4: libxg: drop dead assignment to "rc" from xc_cpuid_apply_policy()
5: libxl: drop dead assignment to transaction variable from libxl__domain_make()

Jan

[PATCH 1/5] xen-mfndump: drop dead assignment to "page" from lookup_pte_func()

[Jan Beulich]

The variable isn't used past the loop, and its value also isn't
meaningful across iterations. Reduce its scope to make this more
obvious.

Coverity ID: 1532310
Fixes: ae763e422430 ("tools/misc: introduce xen-mfndump")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -265,7 +265,6 @@ int lookup_pte_func(int argc, char *argv
 {
     struct xc_domain_meminfo minfo;
     xc_domaininfo_t info;
-    void *page = NULL;
     unsigned long i, j;
     int domid, pte_num;
     xen_pfn_t mfn;
@@ -301,6 +300,8 @@ int lookup_pte_func(int argc, char *argv
 
     for ( i = 0; i < minfo.p2m_size; i++ )
     {
+        void *page;
+
         if ( !(minfo.pfn_type[i] & XEN_DOMCTL_PFINFO_LTABTYPE_MASK) )
             continue;
 
@@ -323,7 +324,6 @@ int lookup_pte_func(int argc, char *argv
         }
 
         munmap(page, XC_PAGE_SIZE);
-        page = NULL;
     }
 
     xc_unmap_domain_meminfo(xch, &minfo);

[PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Jan Beulich]

The variable needs to be properly set only on the error paths.

Coverity ID: 1532311
Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
if the 1st yielded ENOSYS?

--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -1081,13 +1079,12 @@ int libxl__domain_config_setdefault(libx
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
                                          &d_config->c_info.ssidref);
         if (ret) {
-            if (errno == ENOSYS) {
-                LOGD(WARN, domid, "XSM Disabled: init_seclabel not supported");
-                ret = 0;
-            } else {
+            if (errno != ENOSYS) {
                 LOGD(ERROR, domid, "Invalid init_seclabel: %s", s);
                 goto error_out;
             }
+
+            LOGD(WARN, domid, "XSM Disabled: init_seclabel not supported");
         }
     }
 
@@ -1096,13 +1093,12 @@ int libxl__domain_config_setdefault(libx
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
                                          &d_config->b_info.exec_ssidref);
         if (ret) {
-            if (errno == ENOSYS) {
-                LOGD(WARN, domid, "XSM Disabled: seclabel not supported");
-                ret = 0;
-            } else {
+            if (errno != ENOSYS) {
                 LOGD(ERROR, domid, "Invalid seclabel: %s", s);
                 goto error_out;
             }
+
+            LOGD(WARN, domid, "XSM Disabled: seclabel not supported");
         }
     }
 
@@ -1111,14 +1107,13 @@ int libxl__domain_config_setdefault(libx
         ret = libxl_flask_context_to_sid(ctx, s, strlen(s),
                                          &d_config->b_info.device_model_ssidref);
         if (ret) {
-            if (errno == ENOSYS) {
-                LOGD(WARN, domid,
-                     "XSM Disabled: device_model_stubdomain_seclabel not supported");
-                ret = 0;
-            } else {
+            if (errno != ENOSYS) {
                 LOGD(ERROR, domid, "Invalid device_model_stubdomain_seclabel: %s", s);
                 goto error_out;
             }
+
+            LOGD(WARN, domid,
+                 "XSM Disabled: device_model_stubdomain_seclabel not supported");
         }
     }
 

[PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Jan Beulich]

The function returns immediately after the enclosing if().

Coverity ID: 1532314
Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/guest/xg_core_x86.c
+++ b/tools/libs/guest/xg_core_x86.c
@@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
         }
 
         munmap(ptes, n_pages * PAGE_SIZE);
-        ptes = NULL;
         off = p2m_vaddr & ((mask >> shift) << shift);
     }
 

[PATCH 4/5] libxg: drop dead assignment to "rc" from xc_cpuid_apply_policy()

[Jan Beulich]

"rc" is written immediately below the outer if(). Fold the remaining two
if()s.

Coverity ID: 1532320
Fixes: 685e922d6f30 ("tools/libxc: Rework xc_cpuid_apply_policy() to use {get,set}_cpu_policy()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The code in question was subsequently moved by 54463aa79dac ("x86/hvm:
Disable MPX by default").

--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -462,17 +462,12 @@ int xc_cpuid_apply_policy(xc_interface *
     /* Get the host policy. */
     rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
                                &len, host_featureset);
-    if ( rc )
+    /* Tolerate "buffer too small", as we've got the bits we need. */
+    if ( rc && errno != ENOBUFS )
     {
-        /* Tolerate "buffer too small", as we've got the bits we need. */
-        if ( errno == ENOBUFS )
-            rc = 0;
-        else
-        {
-            PERROR("Failed to obtain host featureset");
-            rc = -errno;
-            goto out;
-        }
+        PERROR("Failed to obtain host featureset");
+        rc = -errno;
+        goto out;
     }
 
     /* Get the domain's default policy. */

[PATCH 5/5] libxl: drop dead assignment to transaction variable from libxl__domain_make()

[Jan Beulich]

"t" is written first thing at the "retry_transaction" label.

Coverity ID: 1532321
Fixes: 1057300109ea ("libxl: fix error handling (xenstore transaction leak) in libxl__domain_make")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -909,10 +909,8 @@ retry_transaction:
              strlen(dom_type));
 
     if (!xs_transaction_end(ctx->xsh, t, 0)) {
-        if (errno == EAGAIN) {
-            t = 0;
+        if (errno == EAGAIN)
             goto retry_transaction;
-        }
         LOGED(ERROR, *domid, "domain creation ""xenstore transaction commit failed");
         rc = ERROR_FAIL;
         goto out;

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 464 bytes --]

On 12.06.23 13:46, Jan Beulich wrote:
> The variable needs to be properly set only on the error paths.
> 
> Coverity ID: 1532311
> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>

> ---
> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
> if the 1st yielded ENOSYS?

Probably not.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 341 bytes --]

On 12.06.23 13:46, Jan Beulich wrote:
> The function returns immediately after the enclosing if().
> 
> Coverity ID: 1532314
> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 4/5] libxg: drop dead assignment to "rc" from xc_cpuid_apply_policy()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 367 bytes --]

On 12.06.23 13:47, Jan Beulich wrote:
> "rc" is written immediately below the outer if(). Fold the remaining two
> if()s.
> 
> Coverity ID: 1532320
> Fixes: 685e922d6f30 ("tools/libxc: Rework xc_cpuid_apply_policy() to use {get,set}_cpu_policy()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 5/5] libxl: drop dead assignment to transaction variable from libxl__domain_make()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 347 bytes --]

On 12.06.23 13:47, Jan Beulich wrote:
> "t" is written first thing at the "retry_transaction" label.
> 
> Coverity ID: 1532321
> Fixes: 1057300109ea ("libxl: fix error handling (xenstore transaction leak) in libxl__domain_make")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 1/5] xen-mfndump: drop dead assignment to "page" from lookup_pte_func()

[Jason Andryuk]

On Mon, Jun 12, 2023 at 7:45 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> The variable isn't used past the loop, and its value also isn't
> meaningful across iterations. Reduce its scope to make this more
> obvious.
>
> Coverity ID: 1532310
> Fixes: ae763e422430 ("tools/misc: introduce xen-mfndump")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Thanks,
Jason

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Daniel P. Smith]

On 6/12/23 07:46, Jan Beulich wrote:
> The variable needs to be properly set only on the error paths.
> 
> Coverity ID: 1532311
> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm>

> ---
> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
> if the 1st yielded ENOSYS?

Would you be okay with the calls staying if instead on the first 
invocation of any libxl_flask_* method, flask status was checked and 
stored in a variable that would then be checked by any subsequent calls 
and immediately returned if flask was not enabled?

v/r,
dps

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Daniel P. Smith]

On 6/12/23 15:44, Daniel P. Smith wrote:
> On 6/12/23 07:46, Jan Beulich wrote:
>> The variable needs to be properly set only on the error paths.
>>
>> Coverity ID: 1532311
>> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID 
>> down to libxl")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm>
> 
>> ---
>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>> if the 1st yielded ENOSYS?
> 
> Would you be okay with the calls staying if instead on the first 
> invocation of any libxl_flask_* method, flask status was checked and 
> stored in a variable that would then be checked by any subsequent calls 
> and immediately returned if flask was not enabled?
> 
> v/r,
> dps

Looking closer I realized there is a slight flaw in the logic here. The 
first call is accomplished via an xsm_op call and then assumes that 
FLASK is the only XSM that has implemented the xsm hook, xsm_op, and 
that the result will be an ENOSYS. If someone decides to implement an 
xsm_op hook for any of the existing XSM modules or introduces a new XSM 
module that has an xsm_op hook, the return likely would not be ENOSYS. I 
have often debated if there should be a way to query which XSM module 
was loaded for instances just like this. The question is what mechanism 
would be best to do so.

v/r,
dps

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Jan Beulich]

On 12.06.2023 22:21, Daniel P. Smith wrote:
> 
> 
> On 6/12/23 15:44, Daniel P. Smith wrote:
>> On 6/12/23 07:46, Jan Beulich wrote:
>>> The variable needs to be properly set only on the error paths.
>>>
>>> Coverity ID: 1532311
>>> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID 
>>> down to libxl")
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm>

Thanks.

>>> ---
>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>> if the 1st yielded ENOSYS?
>>
>> Would you be okay with the calls staying if instead on the first 
>> invocation of any libxl_flask_* method, flask status was checked and 
>> stored in a variable that would then be checked by any subsequent calls 
>> and immediately returned if flask was not enabled?

I'm wary of global variables in shared libraries.

> Looking closer I realized there is a slight flaw in the logic here. The 
> first call is accomplished via an xsm_op call and then assumes that 
> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and 
> that the result will be an ENOSYS. If someone decides to implement an 
> xsm_op hook for any of the existing XSM modules or introduces a new XSM 
> module that has an xsm_op hook, the return likely would not be ENOSYS. I 
> have often debated if there should be a way to query which XSM module 
> was loaded for instances just like this. The question is what mechanism 
> would be best to do so.

Well, this is what results from abusing ENOSYS. The default case of a
switch() in a handler shouldn't return that value. Only if the entire
hypercall is outright unimplemented, returning ENOSYS is appropriate.
In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so,
when that function also serves as the fallback for XSM modules
choosing to not implement any of the op-s (like SILO does).

Since in the specific case here it looks like the intention really is
to carry out Flask-specific operations, a means to check for Flask
specifically might indeed be appropriate. If not a new sub-op of
xsm_op, a new sysctl might be another option.

Jan

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Daniel P. Smith]

On 6/13/23 02:33, Jan Beulich wrote:
> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>
>>
>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>> The variable needs to be properly set only on the error paths.
>>>>
>>>> Coverity ID: 1532311
>>>> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID
>>>> down to libxl")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.cm>
> 
> Thanks.
> 

My pleasure.

>>>> ---
>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>> if the 1st yielded ENOSYS?
>>>
>>> Would you be okay with the calls staying if instead on the first
>>> invocation of any libxl_flask_* method, flask status was checked and
>>> stored in a variable that would then be checked by any subsequent calls
>>> and immediately returned if flask was not enabled?
> 
> I'm wary of global variables in shared libraries.
> 

I agree with that sentiment, but I would distinguish global state from a 
global variable. I would take the approach of,

static boot is_flask_enabled(void)
{
     /* 0 unchecked, 1 checked but disabled, 2 enabled */
     static int state = 0;

     if ( state == 0 )
	state = check_flask_state(); /* pseudo call for real logic */

     return (state < 2 ? false : true);
}

Then all the libxl_flask_* methods would is_flask_enabled(). This would 
not expose a global variable that could be manipulated by users of the 
library.


>> Looking closer I realized there is a slight flaw in the logic here. The
>> first call is accomplished via an xsm_op call and then assumes that
>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and
>> that the result will be an ENOSYS. If someone decides to implement an
>> xsm_op hook for any of the existing XSM modules or introduces a new XSM
>> module that has an xsm_op hook, the return likely would not be ENOSYS. I
>> have often debated if there should be a way to query which XSM module
>> was loaded for instances just like this. The question is what mechanism
>> would be best to do so.
> 
> Well, this is what results from abusing ENOSYS. The default case of a
> switch() in a handler shouldn't return that value. Only if the entire
> hypercall is outright unimplemented, returning ENOSYS is appropriate.
> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so,
> when that function also serves as the fallback for XSM modules
> choosing to not implement any of the op-s (like SILO does).

I understand your point, but if ENOSYS (Not Implemented) is not correct, 
what is the appropriate return value for this module does not support 
this op?

> Since in the specific case here it looks like the intention really is
> to carry out Flask-specific operations, a means to check for Flask
> specifically might indeed be appropriate. If not a new sub-op of
> xsm_op, a new sysctl might be another option.

I am actually split on whether this should be an sub-op of xsm op or if 
this should be exposed via hyperfs. I did not consider a sysctl, though 
I guess an argument could be constructed for it.

v/r,
dps

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Jan Beulich]

On 13.06.2023 11:21, Daniel P. Smith wrote:
> On 6/13/23 02:33, Jan Beulich wrote:
>> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>>> if the 1st yielded ENOSYS?
>>>>
>>>> Would you be okay with the calls staying if instead on the first
>>>> invocation of any libxl_flask_* method, flask status was checked and
>>>> stored in a variable that would then be checked by any subsequent calls
>>>> and immediately returned if flask was not enabled?
>>
>> I'm wary of global variables in shared libraries.
>>
> 
> I agree with that sentiment, but I would distinguish global state from a 
> global variable. I would take the approach of,
> 
> static boot is_flask_enabled(void)
> {
>      /* 0 unchecked, 1 checked but disabled, 2 enabled */
>      static int state = 0;
> 
>      if ( state == 0 )
> 	state = check_flask_state(); /* pseudo call for real logic */
> 
>      return (state < 2 ? false : true);
> }
> 
> Then all the libxl_flask_* methods would is_flask_enabled(). This would 
> not expose a global variable that could be manipulated by users of the 
> library.

Well, I certainly did assume the variable wouldn't be widely exposed.
And the library also clearly is far from free of r/w data. But still.

That aspect aside - wouldn't such a basic state determination better
belong in libxc then? (There's far less contents in .data / .bss
there.)

>>> Looking closer I realized there is a slight flaw in the logic here. The
>>> first call is accomplished via an xsm_op call and then assumes that
>>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and
>>> that the result will be an ENOSYS. If someone decides to implement an
>>> xsm_op hook for any of the existing XSM modules or introduces a new XSM
>>> module that has an xsm_op hook, the return likely would not be ENOSYS. I
>>> have often debated if there should be a way to query which XSM module
>>> was loaded for instances just like this. The question is what mechanism
>>> would be best to do so.
>>
>> Well, this is what results from abusing ENOSYS. The default case of a
>> switch() in a handler shouldn't return that value. Only if the entire
>> hypercall is outright unimplemented, returning ENOSYS is appropriate.
>> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so,
>> when that function also serves as the fallback for XSM modules
>> choosing to not implement any of the op-s (like SILO does).
> 
> I understand your point, but if ENOSYS (Not Implemented) is not correct, 
> what is the appropriate return value for this module does not support 
> this op?

You almost say it by the wording you chose: EOPNOTSUPP.

>> Since in the specific case here it looks like the intention really is
>> to carry out Flask-specific operations, a means to check for Flask
>> specifically might indeed be appropriate. If not a new sub-op of
>> xsm_op, a new sysctl might be another option.
> 
> I am actually split on whether this should be an sub-op of xsm op or if 
> this should be exposed via hyperfs. I did not consider a sysctl, though 
> I guess an argument could be constructed for it.

Please don't forget that hypfs, unlike sysctl, is an optional thing,
so you can't use it for decision taking (but only for informational
purposes).

Jan

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Daniel P. Smith]

On 6/13/23 05:40, Jan Beulich wrote:
> On 13.06.2023 11:21, Daniel P. Smith wrote:
>> On 6/13/23 02:33, Jan Beulich wrote:
>>> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>>>> if the 1st yielded ENOSYS?
>>>>>
>>>>> Would you be okay with the calls staying if instead on the first
>>>>> invocation of any libxl_flask_* method, flask status was checked and
>>>>> stored in a variable that would then be checked by any subsequent calls
>>>>> and immediately returned if flask was not enabled?
>>>
>>> I'm wary of global variables in shared libraries.
>>>
>>
>> I agree with that sentiment, but I would distinguish global state from a
>> global variable. I would take the approach of,
>>
>> static boot is_flask_enabled(void)
>> {
>>       /* 0 unchecked, 1 checked but disabled, 2 enabled */
>>       static int state = 0;
>>
>>       if ( state == 0 )
>> 	state = check_flask_state(); /* pseudo call for real logic */
>>
>>       return (state < 2 ? false : true);
>> }
>>
>> Then all the libxl_flask_* methods would is_flask_enabled(). This would
>> not expose a global variable that could be manipulated by users of the
>> library.
> 
> Well, I certainly did assume the variable wouldn't be widely exposed.
> And the library also clearly is far from free of r/w data. But still.
> 
> That aspect aside - wouldn't such a basic state determination better
> belong in libxc then? (There's far less contents in .data / .bss
> there.)

Not opposed at all, so a series with a patch to libxc paired and a new 
sub-op/sysctl as discussed below would be acceptable?

>>>> Looking closer I realized there is a slight flaw in the logic here. The
>>>> first call is accomplished via an xsm_op call and then assumes that
>>>> FLASK is the only XSM that has implemented the xsm hook, xsm_op, and
>>>> that the result will be an ENOSYS. If someone decides to implement an
>>>> xsm_op hook for any of the existing XSM modules or introduces a new XSM
>>>> module that has an xsm_op hook, the return likely would not be ENOSYS. I
>>>> have often debated if there should be a way to query which XSM module
>>>> was loaded for instances just like this. The question is what mechanism
>>>> would be best to do so.
>>>
>>> Well, this is what results from abusing ENOSYS. The default case of a
>>> switch() in a handler shouldn't return that value. Only if the entire
>>> hypercall is outright unimplemented, returning ENOSYS is appropriate.
>>> In fact I wonder whether dummy.h's xsm_do_xsm_op() is validly doing so,
>>> when that function also serves as the fallback for XSM modules
>>> choosing to not implement any of the op-s (like SILO does).
>>
>> I understand your point, but if ENOSYS (Not Implemented) is not correct,
>> what is the appropriate return value for this module does not support
>> this op?
> 
> You almost say it by the wording you chose: EOPNOTSUPP.
> 

Erg, you are right, not sure why it didn't click. Though I guess what 
should be used will be moot if the decision is to add an xsm-op subop to 
dummy to support reporting the current XSM module.

>>> Since in the specific case here it looks like the intention really is
>>> to carry out Flask-specific operations, a means to check for Flask
>>> specifically might indeed be appropriate. If not a new sub-op of
>>> xsm_op, a new sysctl might be another option.
>>
>> I am actually split on whether this should be an sub-op of xsm op or if
>> this should be exposed via hyperfs. I did not consider a sysctl, though
>> I guess an argument could be constructed for it.
> 
> Please don't forget that hypfs, unlike sysctl, is an optional thing,
> so you can't use it for decision taking (but only for informational
> purposes).

Good point regarding hypfs, the check mentioned above is expected to 
always work, thus an optional feature probably is not best.

The next question is, should it be sysctl or xsm-op. I am leaning 
towards xsm-op because as much as I believe XSM should be consider core 
to Xen, the XSM logic should remain contained. Adding it to sysctl would 
mean having to expose XSM state outside of XSM code and would make 
sysctl logic have to be aware of XSM state query functions.

With that said, if someone wants to make the case for sysctl, I am open 
to considering it.

v/r,
dps

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Jan Beulich]

On 13.06.2023 11:57, Daniel P. Smith wrote:
> On 6/13/23 05:40, Jan Beulich wrote:
>> On 13.06.2023 11:21, Daniel P. Smith wrote:
>>> On 6/13/23 02:33, Jan Beulich wrote:
>>>> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>>>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>>>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>>>>> if the 1st yielded ENOSYS?
>>>>>>
>>>>>> Would you be okay with the calls staying if instead on the first
>>>>>> invocation of any libxl_flask_* method, flask status was checked and
>>>>>> stored in a variable that would then be checked by any subsequent calls
>>>>>> and immediately returned if flask was not enabled?
>>>>
>>>> I'm wary of global variables in shared libraries.
>>>>
>>>
>>> I agree with that sentiment, but I would distinguish global state from a
>>> global variable. I would take the approach of,
>>>
>>> static boot is_flask_enabled(void)
>>> {
>>>       /* 0 unchecked, 1 checked but disabled, 2 enabled */
>>>       static int state = 0;
>>>
>>>       if ( state == 0 )
>>> 	state = check_flask_state(); /* pseudo call for real logic */
>>>
>>>       return (state < 2 ? false : true);
>>> }
>>>
>>> Then all the libxl_flask_* methods would is_flask_enabled(). This would
>>> not expose a global variable that could be manipulated by users of the
>>> library.
>>
>> Well, I certainly did assume the variable wouldn't be widely exposed.
>> And the library also clearly is far from free of r/w data. But still.
>>
>> That aspect aside - wouldn't such a basic state determination better
>> belong in libxc then? (There's far less contents in .data / .bss
>> there.)
> 
> Not opposed at all, so a series with a patch to libxc paired and a new 
> sub-op/sysctl as discussed below would be acceptable?

I can only say yes here for the hypervisor side; I'm not a maintainer
of any significant part of the tool stack.

>>>> Since in the specific case here it looks like the intention really is
>>>> to carry out Flask-specific operations, a means to check for Flask
>>>> specifically might indeed be appropriate. If not a new sub-op of
>>>> xsm_op, a new sysctl might be another option.
>>>
>>> I am actually split on whether this should be an sub-op of xsm op or if
>>> this should be exposed via hyperfs. I did not consider a sysctl, though
>>> I guess an argument could be constructed for it.
>>
>> Please don't forget that hypfs, unlike sysctl, is an optional thing,
>> so you can't use it for decision taking (but only for informational
>> purposes).
> 
> Good point regarding hypfs, the check mentioned above is expected to 
> always work, thus an optional feature probably is not best.
> 
> The next question is, should it be sysctl or xsm-op. I am leaning 
> towards xsm-op because as much as I believe XSM should be consider core 
> to Xen, the XSM logic should remain contained. Adding it to sysctl would 
> mean having to expose XSM state outside of XSM code and would make 
> sysctl logic have to be aware of XSM state query functions.

Well, you seemed hesitant towards an xsm-sub-op, so I suggested sysctl
as a possible alternative. One possible issue with an xsm-op is that at
the public interface level (public headers), besides __HYPERVISOR_xsm_op
there's no notion of xsm-op. And it was pointed out before that by kind
of blindly wiring xsm_op through to flask_op, adding XSM-module-
independent ops and/or ops for some future non-Flask module is going to
be a little, well, bumpy.

Jan

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Daniel P. Smith]

On 6/13/23 06:15, Jan Beulich wrote:
> On 13.06.2023 11:57, Daniel P. Smith wrote:
>> On 6/13/23 05:40, Jan Beulich wrote:
>>> On 13.06.2023 11:21, Daniel P. Smith wrote:
>>>> On 6/13/23 02:33, Jan Beulich wrote:
>>>>> On 12.06.2023 22:21, Daniel P. Smith wrote:
>>>>>> On 6/12/23 15:44, Daniel P. Smith wrote:
>>>>>>> On 6/12/23 07:46, Jan Beulich wrote:
>>>>>>>> If XSM is disabled, is it really useful to issue the 2nd and 3rd calls
>>>>>>>> if the 1st yielded ENOSYS?
>>>>>>>
>>>>>>> Would you be okay with the calls staying if instead on the first
>>>>>>> invocation of any libxl_flask_* method, flask status was checked and
>>>>>>> stored in a variable that would then be checked by any subsequent calls
>>>>>>> and immediately returned if flask was not enabled?
>>>>>
>>>>> I'm wary of global variables in shared libraries.
>>>>>
>>>>
>>>> I agree with that sentiment, but I would distinguish global state from a
>>>> global variable. I would take the approach of,
>>>>
>>>> static boot is_flask_enabled(void)
>>>> {
>>>>        /* 0 unchecked, 1 checked but disabled, 2 enabled */
>>>>        static int state = 0;
>>>>
>>>>        if ( state == 0 )
>>>> 	state = check_flask_state(); /* pseudo call for real logic */
>>>>
>>>>        return (state < 2 ? false : true);
>>>> }
>>>>
>>>> Then all the libxl_flask_* methods would is_flask_enabled(). This would
>>>> not expose a global variable that could be manipulated by users of the
>>>> library.
>>>
>>> Well, I certainly did assume the variable wouldn't be widely exposed.
>>> And the library also clearly is far from free of r/w data. But still.
>>>
>>> That aspect aside - wouldn't such a basic state determination better
>>> belong in libxc then? (There's far less contents in .data / .bss
>>> there.)
>>
>> Not opposed at all, so a series with a patch to libxc paired and a new
>> sub-op/sysctl as discussed below would be acceptable?
> 
> I can only say yes here for the hypervisor side; I'm not a maintainer
> of any significant part of the tool stack.
> 

Understood, thank you.

>>>>> Since in the specific case here it looks like the intention really is
>>>>> to carry out Flask-specific operations, a means to check for Flask
>>>>> specifically might indeed be appropriate. If not a new sub-op of
>>>>> xsm_op, a new sysctl might be another option.
>>>>
>>>> I am actually split on whether this should be an sub-op of xsm op or if
>>>> this should be exposed via hyperfs. I did not consider a sysctl, though
>>>> I guess an argument could be constructed for it.
>>>
>>> Please don't forget that hypfs, unlike sysctl, is an optional thing,
>>> so you can't use it for decision taking (but only for informational
>>> purposes).
>>
>> Good point regarding hypfs, the check mentioned above is expected to
>> always work, thus an optional feature probably is not best.
>>
>> The next question is, should it be sysctl or xsm-op. I am leaning
>> towards xsm-op because as much as I believe XSM should be consider core
>> to Xen, the XSM logic should remain contained. Adding it to sysctl would
>> mean having to expose XSM state outside of XSM code and would make
>> sysctl logic have to be aware of XSM state query functions.
> 
> Well, you seemed hesitant towards an xsm-sub-op, so I suggested sysctl
> as a possible alternative. One possible issue with an xsm-op is that at
> the public interface level (public headers), besides __HYPERVISOR_xsm_op
> there's no notion of xsm-op. And it was pointed out before that by kind
> of blindly wiring xsm_op through to flask_op, adding XSM-module-
> independent ops and/or ops for some future non-Flask module is going to
> be a little, well, bumpy.

That is in fact the source of my hesitance. Fixing xsm sub-op from being 
a flask specific, not well documented thing, to a documented general 
interface is on the list of many improvements needed. I believe handling 
this case could be done as an incremental step towards that by 
introducing a common sub-op structure that is used for the "get current 
module" sub-op since it will be new sub-op for FLASK module as well. As 
you mentioned, the next step will be the bumpy one of trying to convert 
the existing FLASK xsm-op sub-ops over to the new common structure. 
Until I try, I won't know for sure if I can do this as a small step. Let 
me see if I can carve out some time and give it a try.

v/r,
dps

Re: [PATCH 1/5] xen-mfndump: drop dead assignment to "page" from lookup_pte_func()

[Anthony PERARD]

On Mon, Jun 12, 2023 at 08:47:41AM -0400, Jason Andryuk wrote:
> On Mon, Jun 12, 2023 at 7:45 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > The variable isn't used past the loop, and its value also isn't
> > meaningful across iterations. Reduce its scope to make this more
> > obvious.
> >
> > Coverity ID: 1532310
> > Fixes: ae763e422430 ("tools/misc: introduce xen-mfndump")
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Jason Andryuk <jandryuk@gmail.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 2/5] libxl: drop dead assignments to "ret" from libxl__domain_config_setdefault()

[Anthony PERARD]

On Mon, Jun 12, 2023 at 01:46:19PM +0200, Jan Beulich wrote:
> The variable needs to be properly set only on the error paths.
> 
> Coverity ID: 1532311
> Fixes: ab4440112bec ("xl / libxl: push parsing of SSID and CPU pool ID down to libxl")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Anthony PERARD]

On Mon, Jun 12, 2023 at 01:46:40PM +0200, Jan Beulich wrote:
> The function returns immediately after the enclosing if().
> 
> Coverity ID: 1532314
> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/tools/libs/guest/xg_core_x86.c
> +++ b/tools/libs/guest/xg_core_x86.c
> @@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
>          }
>  
>          munmap(ptes, n_pages * PAGE_SIZE);
> -        ptes = NULL;
>          off = p2m_vaddr & ((mask >> shift) << shift);
>      }

Do we have to remove this assignment? What if someone adds code later
and reuse the content of the variable `ptes`? Or what if someone adds
codes after the loop, and handle an error with `goto out`, we would have
a double-munmap().

-- 
Anthony PERARD

Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Jan Beulich]

On 13.06.2023 18:03, Anthony PERARD wrote:
> On Mon, Jun 12, 2023 at 01:46:40PM +0200, Jan Beulich wrote:
>> The function returns immediately after the enclosing if().
>>
>> Coverity ID: 1532314
>> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/tools/libs/guest/xg_core_x86.c
>> +++ b/tools/libs/guest/xg_core_x86.c
>> @@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
>>          }
>>  
>>          munmap(ptes, n_pages * PAGE_SIZE);
>> -        ptes = NULL;
>>          off = p2m_vaddr & ((mask >> shift) << shift);
>>      }
> 
> Do we have to remove this assignment? What if someone adds code later
> and reuse the content of the variable `ptes`? Or what if someone adds
> codes after the loop, and handle an error with `goto out`, we would have
> a double-munmap().

Imo it would be at that time when the assignment wants (re)adding. I
fully agree with the tool, and I expect Misra (if it was applied to
the tool stack as well) would demand the very same change.

Jan

Re: [PATCH 4/5] libxg: drop dead assignment to "rc" from xc_cpuid_apply_policy()

[Anthony PERARD]

On Mon, Jun 12, 2023 at 01:47:13PM +0200, Jan Beulich wrote:
> "rc" is written immediately below the outer if(). Fold the remaining two
> if()s.
> 
> Coverity ID: 1532320
> Fixes: 685e922d6f30 ("tools/libxc: Rework xc_cpuid_apply_policy() to use {get,set}_cpu_policy()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 5/5] libxl: drop dead assignment to transaction variable from libxl__domain_make()

[Anthony PERARD]

On Mon, Jun 12, 2023 at 01:47:50PM +0200, Jan Beulich wrote:
> "t" is written first thing at the "retry_transaction" label.
> 
> Coverity ID: 1532321
> Fixes: 1057300109ea ("libxl: fix error handling (xenstore transaction leak) in libxl__domain_make")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Anthony PERARD]

On Tue, Jun 13, 2023 at 06:08:16PM +0200, Jan Beulich wrote:
> On 13.06.2023 18:03, Anthony PERARD wrote:
> > On Mon, Jun 12, 2023 at 01:46:40PM +0200, Jan Beulich wrote:
> >> The function returns immediately after the enclosing if().
> >>
> >> Coverity ID: 1532314
> >> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> --- a/tools/libs/guest/xg_core_x86.c
> >> +++ b/tools/libs/guest/xg_core_x86.c
> >> @@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
> >>          }
> >>  
> >>          munmap(ptes, n_pages * PAGE_SIZE);
> >> -        ptes = NULL;
> >>          off = p2m_vaddr & ((mask >> shift) << shift);
> >>      }
> > 
> > Do we have to remove this assignment? What if someone adds code later
> > and reuse the content of the variable `ptes`? Or what if someone adds
> > codes after the loop, and handle an error with `goto out`, we would have
> > a double-munmap().
> 
> Imo it would be at that time when the assignment wants (re)adding. I

I don't believe this is going to happen because I don't think a compiler
will find a mistake. Maybe a run of Coverity would tell that ptes is
reused after munmap(), but by the time Coverity run on the code, it
would be too late.

> fully agree with the tool, and I expect Misra (if it was applied to
> the tool stack as well) would demand the very same change.

I guess the issue here is that there's two paths out of the function, the
error path via "out" and the success path. If `ptes` is check on both
path, then the assignment would be needed, and it would be harder to
make a mistake; which can be done by having only one way out.

If only we could restrict the scope of `ptes` to the for loop, we
wouldn't even need to set it to NULL.

Cheers,

-- 
Anthony PERARD

Re: [PATCH 3/5] libxg: drop dead assignment to "ptes" from xc_core_arch_map_p2m_list_rw()

[Jan Beulich]

On 13.06.2023 18:40, Anthony PERARD wrote:
> On Tue, Jun 13, 2023 at 06:08:16PM +0200, Jan Beulich wrote:
>> On 13.06.2023 18:03, Anthony PERARD wrote:
>>> On Mon, Jun 12, 2023 at 01:46:40PM +0200, Jan Beulich wrote:
>>>> The function returns immediately after the enclosing if().
>>>>
>>>> Coverity ID: 1532314
>>>> Fixes: bd7a29c3d0b9 ("tools/libs/ctrl: fix xc_core_arch_map_p2m() to support linear p2m table")
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> --- a/tools/libs/guest/xg_core_x86.c
>>>> +++ b/tools/libs/guest/xg_core_x86.c
>>>> @@ -210,7 +210,6 @@ xc_core_arch_map_p2m_list_rw(xc_interfac
>>>>          }
>>>>  
>>>>          munmap(ptes, n_pages * PAGE_SIZE);
>>>> -        ptes = NULL;
>>>>          off = p2m_vaddr & ((mask >> shift) << shift);
>>>>      }
>>>
>>> Do we have to remove this assignment? What if someone adds code later
>>> and reuse the content of the variable `ptes`? Or what if someone adds
>>> codes after the loop, and handle an error with `goto out`, we would have
>>> a double-munmap().
>>
>> Imo it would be at that time when the assignment wants (re)adding. I
> 
> I don't believe this is going to happen because I don't think a compiler
> will find a mistake. Maybe a run of Coverity would tell that ptes is
> reused after munmap(), but by the time Coverity run on the code, it
> would be too late.
> 
>> fully agree with the tool, and I expect Misra (if it was applied to
>> the tool stack as well) would demand the very same change.
> 
> I guess the issue here is that there's two paths out of the function, the
> error path via "out" and the success path. If `ptes` is check on both
> path, then the assignment would be needed, and it would be harder to
> make a mistake; which can be done by having only one way out.
> 
> If only we could restrict the scope of `ptes` to the for loop, we
> wouldn't even need to set it to NULL.

I can do that, but then I can't resist to shrink other variables' scopes
as well, so it'll be somewhat bigger a change. Let's see how you like it.

Jan