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
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