Re: [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, ) < 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  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 
> > > > ---
> > > >  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(, ifname, "operstate") < 0)
> > > >  goto cleanup;
> > > >
> > > > -if (virFileReadAll(path, 1024, ) < 0) {
> > > > +if (virFileReadAllQuiet(path, 1024, ) < 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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list



signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Prerna]

I agree. Thanks for pointing out that this is a behavioural change, which
should not happen.

I should be doing something like this:

if (virFileReadAllQuiet(path, 1024, ) < 0 ) {
if (errno != ENOENT) {
virReportSystemError(errno,
   _("unable to read: %s"), path);

}
   goto cleanup;
}

On Wed, Mar 22, 2017 at 1:56 PM, Peter Krempa  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 
> > > ---
> > >  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(, ifname, "operstate") < 0)
> > >  goto cleanup;
> > >
> > > -if (virFileReadAll(path, 1024, ) < 0) {
> > > +if (virFileReadAllQuiet(path, 1024, ) < 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.
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Peter Krempa]

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 
> > ---
> >  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(, ifname, "operstate") < 0)
> >  goto cleanup;
> >  
> > -if (virFileReadAll(path, 1024, ) < 0) {
> > +if (virFileReadAllQuiet(path, 1024, ) < 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.

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


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Peter Krempa]

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 
> ---
>  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(, ifname, "operstate") < 0)
>  goto cleanup;
>  
> -if (virFileReadAll(path, 1024, ) < 0) {
> +if (virFileReadAllQuiet(path, 1024, ) < 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.


signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[Prerna Saxena]

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 
---
 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(, ifname, "operstate") < 0)
 goto cleanup;
 
-if (virFileReadAll(path, 1024, ) < 0) {
+if (virFileReadAllQuiet(path, 1024, ) < 0 && errno != ENOENT) {
 virReportSystemError(errno,
  _("unable to read: %s"),
  path);
-- 
1.8.1.2

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