On Fri, Jun 17, 2022, 5:49 AM Daniel P. Berrangé <berra...@redhat.com>
wrote:
> 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 <js...@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 <js...@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
>
Sure! I had something like that initially, but chickened out specifically
because I thought major:minor was a nonsense kind of reply, so I opted for
more egregiously obvious nonsense. I figured I'd find strong opinions that
way ;)
I'm just not sure how this data is used in practice so I had no insight as
to what would be best. I can use the basename, sure.
(Should I also add an optional flag field that indicates the path was not
resolvable, do you think? I guess we can always add it later if needed, but
not sure if i need to head that one off at the pass.)
As for Thomas' comment: I wasn't entirely clear on precisely when we'd run
into this scenario and I didn't know if it was a good idea to skip the
entries entirely. Maybe getting platform mount information even if we can't
access it is still important when working with containers? I don't know one
way or the other TBQH. I'm not very well traveled with devices,
filesystems, and permissions where containers are concerned.
/shrug
> 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 :|
>
>
