Re: [PATCH] qga: implement 'guest-get-nvidia-smi' command

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Apr 01, 2026 at 02:50:31PM +0300, Kostiantyn Kostiuk wrote:
> On Wed, Apr 1, 2026 at 2:25 PM Markus Armbruster <armbru@redhat.com> wrote:
> 
> > You neglected to cc: me.  We recommend to use scripts/get_maintainer.pl
> > to find all the maintainers, then use common sense to trim.
> >
> > João Vilaça <machadovilaca@gmail.com> writes:
> >
> > The commit message needs to explain why and how the patch is useful.
> >
> > For a patch adding a command to qemu-ga, like this one, it needs to
> > state the command's anticipated use cases.
> >
> > > ---
> > >  qga/commands-posix.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> > >  qga/commands-win32.c | 64 ++++++++++++++++++++++++++++++++++++++++++++
> > >  qga/qapi-schema.json | 59 ++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 187 insertions(+)
> > >
> > > diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> > > index 837be51c40..631a8a9ee6 100644
> > > --- a/qga/commands-posix.c
> > > +++ b/qga/commands-posix.c
> > > @@ -1415,3 +1415,67 @@ GuestLoadAverage *qmp_guest_get_load(Error **errp)
> > >      return ret;
> > >  }
> > >  #endif
> > > +
> > > +GuestNvidiaGpuList *qmp_guest_get_nvidia_smi(Error **errp)
> > > +{
> > > +    const gchar *argv[] = {
> > > +        "nvidia-smi",
> > > +        "--query-gpu=index,name,driver_version,"
> > > +            "temperature.gpu,utilization.gpu,utilization.memory,"
> > > +            "memory.total,memory.free,memory.used",
> > > +        "--format=csv,noheader,nounits",
> > > +        NULL
> > > +    };
> > > +    g_autofree gchar *stdout_buf = NULL;
> > > +    g_autofree gchar *stderr_buf = NULL;
> > > +    gint exit_status;
> > > +    GError *gerr = NULL;
> > > +    GuestNvidiaGpuList *head = NULL, **tail = &head;
> > > +
> > > +    if (!g_spawn_sync(NULL, (gchar **)argv, NULL,
> > > +                      G_SPAWN_SEARCH_PATH,
> > > +                      NULL, NULL,
> > > +                      &stdout_buf, &stderr_buf,
> > > +                      &exit_status, &gerr)) {
> >
> > Why not ga_run_command()?  Hmm, it throws away the command's output on
> > success.
> >
> > Kostiantyn, should ga_run_command() be rewritten on top of
> > g_spawn_sync()?
> >
> 
> The idea of ga_run_command was different.
> Now, we can improve it if we see new use cases.

It looks pretty simple to modify the existing ga_run_command to
return stdout &stderr as buffers, rather than discarding on
success.

Re-writing to use g_spawn_sync would not be required for that,
though it might be a nice thing todo at some point for platform
portability.


> > > +#
> > > +# Since: 10.1
> > > +##
> > > +{ 'command': 'guest-get-nvidia-smi',
> > > +  'returns': ['GuestNvidiaGpu'] }
> > > +
> > >  ##
> > >  # @GuestNetworkRoute:
> > >  #
> >
> > Why not use existing guest-exec, and leave the parsing to the client?
> >
> 
> I agree with Daniel that guest-exec is a command that should not exist.
> For example, in RHEL/CentOS, this command is disabled by default, and
> we recommend using SSH over VSock to run any command.
> We have seen the problem several times with guest-exec. Also, we have an
> upstream patch
> for guest-exec + SELinux support to try to workaround a security issue.
> Maybe we should discuss marking guest-exec as deprecated.
> 
> So, I agree with the author to add a separate command instead.

I'm somewhat sceptical that any of this should be in scope for the
QEMU agent, as opposed to being left as a job for an existing
general purpose monitoring agent.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|