| Message ID | 20240909031736.1881-1-andrew.yuan@jaguarmicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove unnecessary code in the interface accel_system_init_ops_interfaces | expand |
On 9/9/24 12:17 AM, Andrew.Yuan wrote: > The code 'ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));' is unnecessary; > > And, the following code : > 1.has the same functionality; > 2.includes error checking; > > Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com> > --- > accel/accel-system.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/accel/accel-system.c b/accel/accel-system.c > index f6c947dd82..5d502c8fd8 100644 > --- a/accel/accel-system.c > +++ b/accel/accel-system.c > @@ -73,7 +73,7 @@ void accel_system_init_ops_interfaces(AccelClass *ac) > g_assert(ac_name != NULL); > > ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name); > - ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name)); > + The code you're changing was added by 5141e9a23f ("accel: abort if we fail to load the accelerator plugin") and I think this repetition is intended. If I have to guess (first time looking at this code), ACCEL_OPS_CLASS() is creating the class type QOM functions that the the second module_object_class_by_name() relies on to catch the module load error the commit is trying to address. I'm CCing Claudio to get a better idea of the intention here. At the very least we should add a code comment explaining the reasoning behind initing 'ops' two times in a row and so on. Thanks, Daniel > oc = module_object_class_by_name(ops_name); > if (!oc) { > error_report("fatal: could not load module for type '%s'", ops_name);
On 9/9/24 11:54, Daniel Henrique Barboza wrote: > > > On 9/9/24 12:17 AM, Andrew.Yuan wrote: >> The code 'ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));' is unnecessary; >> >> And, the following code : >> 1.has the same functionality; >> 2.includes error checking; >> >> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com> >> --- >> accel/accel-system.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/accel/accel-system.c b/accel/accel-system.c >> index f6c947dd82..5d502c8fd8 100644 >> --- a/accel/accel-system.c >> +++ b/accel/accel-system.c >> @@ -73,7 +73,7 @@ void accel_system_init_ops_interfaces(AccelClass *ac) >> g_assert(ac_name != NULL); >> >> ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name); >> - ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name)); >> + > > The code you're changing was added by 5141e9a23f ("accel: abort if we fail to > load the accelerator plugin") and I think this repetition is intended. If I have > to guess (first time looking at this code), ACCEL_OPS_CLASS() is creating the class > type QOM functions that the the second module_object_class_by_name() relies on to > catch the module load error the commit is trying to address. > > I'm CCing Claudio to get a better idea of the intention here. At the very least we > should add a code comment explaining the reasoning behind initing 'ops' two times > in a row and so on. > > > Thanks, > > Daniel Hi Daniel, just to signal that I've seen this message and will get to it when I am back to work later this week. Ciao, Claudio > >> oc = module_object_class_by_name(ops_name); >> if (!oc) { >> error_report("fatal: could not load module for type '%s'", ops_name);
On 9/9/24 12:07, Claudio Fontana wrote: > On 9/9/24 11:54, Daniel Henrique Barboza wrote: >> >> >> On 9/9/24 12:17 AM, Andrew.Yuan wrote: >>> The code 'ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));' is unnecessary; >>> >>> And, the following code : >>> 1.has the same functionality; >>> 2.includes error checking; >>> >>> Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com> >>> --- >>> accel/accel-system.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/accel/accel-system.c b/accel/accel-system.c >>> index f6c947dd82..5d502c8fd8 100644 >>> --- a/accel/accel-system.c >>> +++ b/accel/accel-system.c >>> @@ -73,7 +73,7 @@ void accel_system_init_ops_interfaces(AccelClass *ac) >>> g_assert(ac_name != NULL); >>> >>> ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name); >>> - ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name)); >>> + >> >> The code you're changing was added by 5141e9a23f ("accel: abort if we fail to >> load the accelerator plugin") and I think this repetition is intended. If I have >> to guess (first time looking at this code), ACCEL_OPS_CLASS() is creating the class >> type QOM functions that the the second module_object_class_by_name() relies on to >> catch the module load error the commit is trying to address. >> >> I'm CCing Claudio to get a better idea of the intention here. At the very least we >> should add a code comment explaining the reasoning behind initing 'ops' two times >> in a row and so on. >> >> >> Thanks, >> >> Daniel > > Hi Daniel, just to signal that I've seen this message and will get to it when I am back to work later this week. > > Ciao, > > Claudio > Hi all, I think it was my mistake. I already detected it during the PULL request, but my message was missed at the time I think: https://lists.gnu.org/archive/html/qemu-devel/2022-11/msg01056.html So Reviewed-by: Claudio Fontana <cfontana@suse.de> Thanks, CLaudio
diff --git a/accel/accel-system.c b/accel/accel-system.c index f6c947dd82..5d502c8fd8 100644 --- a/accel/accel-system.c +++ b/accel/accel-system.c @@ -73,7 +73,7 @@ void accel_system_init_ops_interfaces(AccelClass *ac) g_assert(ac_name != NULL); ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name); - ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name)); + oc = module_object_class_by_name(ops_name); if (!oc) { error_report("fatal: could not load module for type '%s'", ops_name);
The code 'ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));' is unnecessary; And, the following code : 1.has the same functionality; 2.includes error checking; Signed-off-by: Andrew.Yuan <andrew.yuan@jaguarmicro.com> --- accel/accel-system.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)