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

-----Original Message----- From: Wang, Huaqiang Sent: Tuesday, October 9, 2018 5:55 PM To: 'Ján Tomko' <jtomko(a)redhat.com>; John Ferlan <jferlan(a)redhat.com&gt; Cc: libvir-list(a)redhat.com; Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; Ding, Jian-feng <jian-feng.ding(a)intel.com>; Zang, Rui <rui.zang(a)intel.com&gt; Subject: RE: [libvirt] [PATCHv2 1/4] util: Introduce monitor capability interface > -----Original Message----- > From: Ján Tomko [mailto:jtomko@redhat.com] > Sent: Friday, October 5, 2018 10:42 PM > To: John Ferlan <jferlan(a)redhat.com&gt; > Cc: Wang, Huaqiang <huaqiang.wang(a)intel.com>; libvir-list(a)redhat.com; > Feng, Shaohe <shaohe.feng(a)intel.com>; Niu, Bing <bing.niu(a)intel.com>; > Ding, Jian- feng <jian-feng.ding(a)intel.com>; Zang, Rui > <rui.zang(a)intel.com&gt; > 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(a)intel.com&gt; > >> --- > >> 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;