Re: [PATCH 03/15] vbox: Drop @iid from UIMachine::LaunchVMProcess()

On 1/23/23 11:00, Martin Kletzander wrote: > On Mon, Jan 23, 2023 at 10:34:12AM +0100, Michal Privoznik wrote: >> The @iid argument of UIMachine::LaunchVMProcess() callback is >> unused. Drop it and also its propagation from parent functions. >> > > Looks like even in 6.1 this was not a function parameter of > LaunchVMProcess.  Was it ever? Looks like it was. Commit v1.2.8-rc1~155 suggests it was used for VBox < 4.0. > What I'm trying to figure out is whether > there is a way to make sure other cases don't happen or are not present > in the code already. I don't think there's an algorithmic way to check that. What I did in this series is drop all G_GNUC_UNUSED, let compiler warn me about unused arguments and then either put the flag back OR drop the argument if it's unused. My reasoning was that with ever-changing VBox API those unused arguments are leftovers from previous version of APIs (but I might be wrong).

> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/vbox/vbox_common.c        | 6 +++--- >> src/vbox/vbox_tmpl.c          | 1 - >> src/vbox/vbox_uniformed_api.h | 1 - >> 3 files changed, 3 insertions(+), 5 deletions(-) >> >> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c >> index ea3a54b7c9..7b1b8bb1b0 100644 >> --- a/src/vbox/vbox_common.c >> +++ b/src/vbox/vbox_common.c >> @@ -2046,7 +2046,7 @@ static int vboxDomainUndefine(virDomainPtr dom) >> } >> >> static int >> -vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine, >> vboxIID *iid) >> +vboxStartMachine(virDomainPtr dom, int maxDomID, IMachine *machine) >> { >>     struct _vboxDriver *data = dom->conn->privateData; >>     int vrdpPresent = 0; >> @@ -2147,7 +2147,7 @@ vboxStartMachine(virDomainPtr dom, int maxDomID, >> IMachine *machine, vboxIID *iid >>     if (vrdpPresent) >>         VBOX_UTF8_TO_UTF16("vrdp", &sessionType); >> >> -    rc = gVBoxAPI.UIMachine.LaunchVMProcess(data, machine, iid, >> +    rc = gVBoxAPI.UIMachine.LaunchVMProcess(data, machine, >>                                             sessionType, env, >>                                             &progress); >> >> @@ -2238,7 +2238,7 @@ static int >> vboxDomainCreateWithFlags(virDomainPtr dom, unsigned int flags) >>                 gVBoxAPI.UIMachine.GetState(machine, &state); >> >>                 if (gVBoxAPI.machineStateChecker.NotStart(state)) { >> -                    ret = vboxStartMachine(dom, i, machine, &iid); >> +                    ret = vboxStartMachine(dom, i, machine); > > Looks like this makes the iid unused in this function (or rather code > block) and can be removed too. I'm not so sure about that. There's a for() loop that traverses through all machines, trying to find the one that the function was called with and this is done by comparing iid (which in fact is domain UUID, not domain ID as we understand it). Or am I missing something?

Michal