Hi Jano, Recognized there was already a commit for a fixing: 7bff646d71aa90ed8727ef99be29d6d2ab5d8f06. And now I got your idea. Thanks Huaqiang > -----Original Message----- > From: Wang, Huaqiang > Sent: Tuesday, October 9, 2018 5:55 PM > To: 'Ján Tomko' <jtomko@xxxxxxxxxx>; John Ferlan <jferlan@xxxxxxxxxx> > Cc: libvir-list@xxxxxxxxxx; Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing > <bing.niu@xxxxxxxxx>; Ding, Jian-feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui > <rui.zang@xxxxxxxxx> > Subject: RE: [PATCHv2 1/4] util: Introduce monitor capability interface > > > > > -----Original Message----- > > From: Ján Tomko [mailto:jtomko@xxxxxxxxxx] > > Sent: Friday, October 5, 2018 10:42 PM > > To: John Ferlan <jferlan@xxxxxxxxxx> > > Cc: Wang, Huaqiang <huaqiang.wang@xxxxxxxxx>; libvir-list@xxxxxxxxxx; > > Feng, Shaohe <shaohe.feng@xxxxxxxxx>; Niu, Bing <bing.niu@xxxxxxxxx>; > > Ding, Jian- feng <jian-feng.ding@xxxxxxxxx>; Zang, Rui > > <rui.zang@xxxxxxxxx> > > Subject: Re: [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@xxxxxxxxx> > > >> --- > > >> 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; -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
