[libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface

[Wang, Huaqiang]

> -----Original Message-----
> From: Ján Tomko [mailto:jtomko at redhat.com]
> Sent: Friday, October 5, 2018 10:42 PM
> To: John Ferlan <jferlan at redhat.com>
> Cc: Wang, Huaqiang <huaqiang.wang at intel.com>; libvir-list at redhat.com; Feng,
> Shaohe <shaohe.feng at intel.com>; Niu, Bing <bing.niu at intel.com>; Ding, Jian-
> feng <jian-feng.ding at intel.com>; Zang, Rui <rui.zang at intel.com>
> Subject: Re: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface
> 
> On Tue, Sep 18, 2018 at 03:38:44PM -0400, John Ferlan wrote:
> >On 09/14/2018 09:30 PM, Wang Huaqiang wrote:
> >> This patch introduces the resource monitor and creates the interface
> >> for getting host capability of resource monitor from the system
> >> resource control file system.
> >>
> >> The resource monitor take the role of RDT monitoring group, could be
> >
> >*takes...
> >
> >s/, could/ and could/
> >
> >> used to monitor the resource consumption information, such as the
> >> last level cache occupancy and the utilization of memory bandwidth.
> >>
> >> Signed-off-by: Wang Huaqiang <huaqiang.wang at intel.com>
> >> ---
> >>  src/util/virresctrl.c | 124
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 124 insertions(+)
> >>
> 
> [...]
> 
> >> +
> >> +    rv = virFileReadValueUint(&info_monitor->max_monitor,
> >> +                              SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
> >> +    if (rv == -2) {
> >> +        /* The file doesn't exist, so it's unusable for us, probably resource
> >> +         * monitor unsupported */
> >> +        VIR_INFO("The path '" SYSFS_RESCTRL_PATH
> "/info/L3_MON/num_rmids' "
> >> +                 "does not exist");
> >
> >Add virResetLastError()
> >
> >[avoids having this error in Last and something else failing and
> >spewing the error]
> >
> 
> The return value of -2 means no error was set, so there is nothing to do here.

A return value of -2 means no error, rather than executing remaining part of function,
it requires to return to the caller without reporting any error.

Here, a return value of -2 means "/info/L3_MON/num_rmids" is not exists, this
could happen if CMT is not supported by host. This is a valid scenario and does
not mean an error, and this function should not report any error to its caller, and
the caller, which is virResctrlGetInfo, will continue to run its remaining statements
normally.

> 
> Also, virResetLastError is meant to be used before starting an API.
> It only resets the thread-local error object (which can only contain one error), it
> cannot possibly unlog an error that was logged earlier.
> In that case, creating a Quiet version of the function is the proper solution.

Do you mean the message reported by 'VIR_INFO' should be removed and also not
adding the 'virResetLastError' line?

It might be more consistent if we keep the 'VIR_INFO' lines, because the similar
message has been emitted in checking the memory bandwidth information and
cache allocation information. But if you insist that this message should not be
shown to user or developer, I could accept that and make change to it.

@@ -704,12 +704,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl)
     rv = virFileReadValueUint(&info_monitor->max_monitor,
                               SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids");
     if (rv == -2) {
        /* The file doesn't exist, so it's unusable for us, probably resource
         * monitor unsupported */
        VIR_INFO("The file '" SYSFS_RESCTRL_PATH "/info/L3_MON/num_rmids' "
                 "does not exist");
         ret = 0;
-        virResetLastError();
         goto cleanup;
     } else if (rv < 0) {
         /* Other failures are fatal, so just quit */

> 
> Jano
> 
> >> +        ret = 0;
> >> +        goto cleanup;