Re: [PATCH v2 03/10] qga: treat get-guest-fsinfo as "best effort"

[Daniel P . Berrangé]

On Thu, Jun 16, 2022 at 06:35:44PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Jun 16, 2022 at 6:27 PM John Snow <jsnow@redhat.com> wrote:
> 
> > In some container environments, there may be references to block devices
> > witnessable from a container through /proc/self/mountinfo that reference
> > devices we simply don't have access to in the container, and could not
> > provide information about.
> >
> > Instead of failing the entire fsinfo command, return stub information
> > for these failed lookups.
> >
> > This allows test-qga to pass under docker tests, which are in turn used
> > by the CentOS VM tests.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  qga/commands-posix.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > index 0469dc409d4..5989d4dca9d 100644
> > --- a/qga/commands-posix.c
> > +++ b/qga/commands-posix.c
> > @@ -1207,7 +1207,13 @@ static void build_guest_fsinfo_for_device(char
> > const *devpath,
> >
> >      syspath = realpath(devpath, NULL);
> >      if (!syspath) {
> > -        error_setg_errno(errp, errno, "realpath(\"%s\")", devpath);
> > +        if (errno == ENOENT) {
> > +            /* This devpath may not exist because of container config,
> > etc. */
> > +            fprintf(stderr, "realpath(%s) returned NULL/ENOENT\n",
> > devpath);
> >
> 
> qga uses g_critical() (except for some win32 code paths atm)
> 
> 
> > +            fs->name = y
> >
> 
> Hmm, maybe we should make the field optional instead.

In my own testing, this method is called in various scenarios.
Some example:

  devpath==/sys/dev/block/253:0
  syspath==/sys/devices/virtual/block/dm-0

    => fs->name == dm-0

  devpath==/sys/devices/virtual/block/dm-0/slaves/nvme0n1p4
  
syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p4

    => fs->name == nvme0n1p4

  devpath==/sys/dev/block/259:2
  
syspath==/sys/devices/pci0000:00/0000:00:1d.0/0000:02:00.0/nvme/nvme0/nvme0n1/nvme0n1p2

    => fs->name == nvme0n1p2

We set fs->name from  basename(syspath)

If the realpath call fails, we could use  basename(devpath). That
would sometimes give the correct answer, and in other types it
would at least give the major:minor number, which an admin can
manually correlate if desired via /proc/partitions.

If we want to be really advanced, we could just open /proc/partitions
and resolve the proper name ourselves, but that's probably overkill

  basename(sysfspath)

is better than g_strdup("??\?-ENOENT")  IMHO

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|