[libvirt] [PATCH 2/3] Debug: Remove unnecessary errors reported while parsing non-existent sysfs files.

[Peter Krempa]

On Wed, Mar 22, 2017 at 14:13:25 +0530, Prerna wrote:
> I agree. Thanks for pointing out that this is a behavioural change, which
> should not happen.

Please don't top post on technical lists.

> 
> I should be doing something like this:
> 
> if (virFileReadAllQuiet(path, 1024, &buf) < 0 ) {
>     if (errno != ENOENT) {
>         virReportSystemError(errno,
>                                    _("unable to read: %s"), path);
> 
>     }
>    goto cleanup;
> }

No, that still would break at least the one case I've pointed out below
[1].

You need to make sure that callers are okay with such change. Or you
need to find out the code path to the message you want to get rid of.
But doing this chagne without regards to odther code paths is not the
right approach.

> 
> On Wed, Mar 22, 2017 at 1:56 PM, Peter Krempa <pkrempa at redhat.com> wrote:
> 
> > On Wed, Mar 22, 2017 at 09:14:41 +0100, Peter Krempa wrote:
> > > On Wed, Mar 22, 2017 at 01:02:18 -0700, Prerna Saxena wrote:
> > > > Sample from current logs:
> > > > error : virFileReadAll:1290 : Failed to open file '/sys/class/net/tap3/operstate':
> > No such file or directory
> > > > error : virNetDevGetLinkInfo:1895 : unable to read:
> > /sys/class/net/tap3/operstate: No such file or directory
> > > >
> > > > These have no useful data point and are redundant.
> > > >
> > > > Signed-off-by: Prerna Saxena <saxenap.ltc at gmail.com>
> > > > ---
> > > >  src/util/virnetdev.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c
> > > > index d123248..3e2f962 100644
> > > > --- a/src/util/virnetdev.c
> > > > +++ b/src/util/virnetdev.c
> > > > @@ -1874,7 +1874,7 @@ virNetDevGetLinkInfo(const char *ifname,
> > > >      if (virNetDevSysfsFile(&path, ifname, "operstate") < 0)
> > > >          goto cleanup;
> > > >
> > > > -    if (virFileReadAll(path, 1024, &buf) < 0) {
> > > > +    if (virFileReadAllQuiet(path, 1024, &buf) < 0 && errno != ENOENT)
> > {
> > > >          virReportSystemError(errno,
> > > >                               _("unable to read: %s"),
> > > >                               path);
> > >
> > > Remove this message here instead of switching to virFileReadAllQuiet.
> > > virFileReadAll reports messages according to the failure itself.
> >
> > Hmm, So you want to avoid the error message altogether. So in that case
> > you should rather call virFileAccess and also note what happens in an
> > comment.
> >
> > Additionally since that would be a semantic change you need to mention
> > it in the commit message and also make sure that all callers would
> > ignore such failure. Otherwise you may cause a regression where we'd
> > report an "unknown error" in cases where it was actually necessary.
> >

[1]

> > Since I found at least one code path that propagates the error
> > (nodeDeviceGetXMLDesc) you should re-evaluate your approach.
> >

> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170322/83763cc4/attachment-0001.sig>