| Message ID | 20210422161130.652779-12-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386: KVM: expand Hyper-V features early | expand |
On Thu, Apr 22, 2021 at 06:11:22PM +0200, Vitaly Kuznetsov wrote: > Use standard error_setg() mechanism in hyperv_expand_features(). > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> No objections, but only suggestions below: > --- > target/i386/kvm/kvm.c | 101 +++++++++++++++++++++++++----------------- > 1 file changed, 61 insertions(+), 40 deletions(-) > > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index a2ef2dc154a2..f33ba325187f 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -1135,7 +1135,7 @@ static bool hyperv_feature_supported(CPUState *cs, int feature) > return true; > } > > -static int hv_cpuid_check_and_set(CPUState *cs, int feature) > +static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp) If changing the function signature, and the function only returns 0 or 1, maybe it's a good opportunity to change to a bool return value format? From include/qapi/error.h: * - Whenever practical, also return a value that indicates success / * failure. This can make the error checking more concise, and can * avoid useless error object creation and destruction. Note that * we still have many functions returning void. We recommend * • bool-valued functions return true on success / false on failure, * • pointer-valued functions return non-null / null pointer, and * • integer-valued functions return non-negative / negative. > { > X86CPU *cpu = X86_CPU(cs); > uint64_t deps; > @@ -1149,20 +1149,18 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature) > while (deps) { > dep_feat = ctz64(deps); > if (!(hyperv_feat_enabled(cpu, dep_feat))) { > - fprintf(stderr, > - "Hyper-V %s requires Hyper-V %s\n", > - kvm_hyperv_properties[feature].desc, > - kvm_hyperv_properties[dep_feat].desc); > - return 1; > + error_setg(errp, "Hyper-V %s requires Hyper-V %s", > + kvm_hyperv_properties[feature].desc, > + kvm_hyperv_properties[dep_feat].desc); > + return 1; > } > deps &= ~(1ull << dep_feat); > } > > if (!hyperv_feature_supported(cs, feature)) { > if (hyperv_feat_enabled(cpu, feature)) { > - fprintf(stderr, > - "Hyper-V %s is not supported by kernel\n", > - kvm_hyperv_properties[feature].desc); > + error_setg(errp, "Hyper-V %s is not supported by kernel", > + kvm_hyperv_properties[feature].desc); > return 1; > } else { > return 0; > @@ -1209,13 +1207,12 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg) > * of 'hv_passthrough' mode and fills the environment with all supported > * Hyper-V features. > */ > -static int hyperv_expand_features(CPUState *cs) > +static void hyperv_expand_features(CPUState *cs, Error **errp) Same as above: returning a value to indicate error is preferred. If you are no longer returning an integer error code, I suggest returning bool instead. > { > X86CPU *cpu = X86_CPU(cs); > - int r; > > if (!hyperv_enabled(cpu)) > - return 0; > + return; > > if (cpu->hyperv_passthrough) { > cpu->hyperv_vendor_id[0] = > @@ -1262,37 +1259,60 @@ static int hyperv_expand_features(CPUState *cs) > } > > /* Features */ > - r = hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI); > - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT); > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) { > + return; > + } What about a loop? for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) { if (hv_cpuid_check_and_set(cs, feat, errp)) { return; } } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) { > + return; > + } > + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) { > + return; > + } > > /* Additional dependencies not covered by kvm_hyperv_properties[] */ > if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) && > !cpu->hyperv_synic_kvm_only && > !hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX)) { > - fprintf(stderr, "Hyper-V %s requires Hyper-V %s\n", > - kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, > - kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); > - r |= 1; > - } > - > - if (r) { > - return -ENOSYS; > + error_setg(errp, "Hyper-V %s requires Hyper-V %s", > + kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, > + kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); > } > - > - return 0; > } > > /* > @@ -1527,9 +1547,10 @@ int kvm_arch_init_vcpu(CPUState *cs) > env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY; > > /* Paravirtualization CPUIDs */ > - r = hyperv_expand_features(cs); > - if (r < 0) { > - return r; > + hyperv_expand_features(cs, &local_err); > + if (local_err) { > + error_report_err(local_err); > + return -ENOSYS; > } > > if (hyperv_enabled(cpu)) { I don't want to block this series because of the suggestions above, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> But I still encourage you to implement those suggestions, anyway.
Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Apr 22, 2021 at 06:11:22PM +0200, Vitaly Kuznetsov wrote: >> Use standard error_setg() mechanism in hyperv_expand_features(). >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > No objections, but only suggestions below: > >> --- >> target/i386/kvm/kvm.c | 101 +++++++++++++++++++++++++----------------- >> 1 file changed, 61 insertions(+), 40 deletions(-) >> >> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c >> index a2ef2dc154a2..f33ba325187f 100644 >> --- a/target/i386/kvm/kvm.c >> +++ b/target/i386/kvm/kvm.c >> @@ -1135,7 +1135,7 @@ static bool hyperv_feature_supported(CPUState *cs, int feature) >> return true; >> } >> >> -static int hv_cpuid_check_and_set(CPUState *cs, int feature) >> +static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp) > > If changing the function signature, and the function only returns 0 or 1, maybe > it's a good opportunity to change to a bool return value format? > > From include/qapi/error.h: > > * - Whenever practical, also return a value that indicates success / > * failure. This can make the error checking more concise, and can > * avoid useless error object creation and destruction. Note that > * we still have many functions returning void. We recommend > * • bool-valued functions return true on success / false on failure, > * • pointer-valued functions return non-null / null pointer, and > * • integer-valued functions return non-negative / negative. > > >> { >> X86CPU *cpu = X86_CPU(cs); >> uint64_t deps; >> @@ -1149,20 +1149,18 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature) >> while (deps) { >> dep_feat = ctz64(deps); >> if (!(hyperv_feat_enabled(cpu, dep_feat))) { >> - fprintf(stderr, >> - "Hyper-V %s requires Hyper-V %s\n", >> - kvm_hyperv_properties[feature].desc, >> - kvm_hyperv_properties[dep_feat].desc); >> - return 1; >> + error_setg(errp, "Hyper-V %s requires Hyper-V %s", >> + kvm_hyperv_properties[feature].desc, >> + kvm_hyperv_properties[dep_feat].desc); >> + return 1; >> } >> deps &= ~(1ull << dep_feat); >> } >> >> if (!hyperv_feature_supported(cs, feature)) { >> if (hyperv_feat_enabled(cpu, feature)) { >> - fprintf(stderr, >> - "Hyper-V %s is not supported by kernel\n", >> - kvm_hyperv_properties[feature].desc); >> + error_setg(errp, "Hyper-V %s is not supported by kernel", >> + kvm_hyperv_properties[feature].desc); >> return 1; >> } else { >> return 0; >> @@ -1209,13 +1207,12 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg) >> * of 'hv_passthrough' mode and fills the environment with all supported >> * Hyper-V features. >> */ >> -static int hyperv_expand_features(CPUState *cs) >> +static void hyperv_expand_features(CPUState *cs, Error **errp) > > Same as above: returning a value to indicate error is preferred. If you are no > longer returning an integer error code, I suggest returning bool instead. > hv_cpuid_check_and_set() is eliminated later in the series but hyperv_expand_features() stays, I can make it bool. >> { >> X86CPU *cpu = X86_CPU(cs); >> - int r; >> >> if (!hyperv_enabled(cpu)) >> - return 0; >> + return; >> >> if (cpu->hyperv_passthrough) { >> cpu->hyperv_vendor_id[0] = >> @@ -1262,37 +1259,60 @@ static int hyperv_expand_features(CPUState *cs) >> } >> >> /* Features */ >> - r = hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI); >> - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT); >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) { >> + return; >> + } > > What about a loop? > > for (feat = 0; feat < ARRAY_SIZE(kvm_hyperv_properties); feat++) { > if (hv_cpuid_check_and_set(cs, feat, errp)) { > return; > } > } > This is done later in the series ("i386: kill off hv_cpuid_check_and_set()"). >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) { >> + return; >> + } >> + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) { >> + return; >> + } >> >> /* Additional dependencies not covered by kvm_hyperv_properties[] */ >> if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) && >> !cpu->hyperv_synic_kvm_only && >> !hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX)) { >> - fprintf(stderr, "Hyper-V %s requires Hyper-V %s\n", >> - kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, >> - kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); >> - r |= 1; >> - } >> - >> - if (r) { >> - return -ENOSYS; >> + error_setg(errp, "Hyper-V %s requires Hyper-V %s", >> + kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, >> + kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); >> } >> - >> - return 0; >> } >> >> /* >> @@ -1527,9 +1547,10 @@ int kvm_arch_init_vcpu(CPUState *cs) >> env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY; >> >> /* Paravirtualization CPUIDs */ >> - r = hyperv_expand_features(cs); >> - if (r < 0) { >> - return r; >> + hyperv_expand_features(cs, &local_err); >> + if (local_err) { >> + error_report_err(local_err); >> + return -ENOSYS; >> } >> >> if (hyperv_enabled(cpu)) { > > I don't want to block this series because of the suggestions above, so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > But I still encourage you to implement those suggestions, anyway. 'Loop' idea is already implemented and hv_cpuid_check_and_set() is gone but I'll remember to make hyperv_expand_features() bool. Thanks!
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index a2ef2dc154a2..f33ba325187f 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -1135,7 +1135,7 @@ static bool hyperv_feature_supported(CPUState *cs, int feature) return true; } -static int hv_cpuid_check_and_set(CPUState *cs, int feature) +static int hv_cpuid_check_and_set(CPUState *cs, int feature, Error **errp) { X86CPU *cpu = X86_CPU(cs); uint64_t deps; @@ -1149,20 +1149,18 @@ static int hv_cpuid_check_and_set(CPUState *cs, int feature) while (deps) { dep_feat = ctz64(deps); if (!(hyperv_feat_enabled(cpu, dep_feat))) { - fprintf(stderr, - "Hyper-V %s requires Hyper-V %s\n", - kvm_hyperv_properties[feature].desc, - kvm_hyperv_properties[dep_feat].desc); - return 1; + error_setg(errp, "Hyper-V %s requires Hyper-V %s", + kvm_hyperv_properties[feature].desc, + kvm_hyperv_properties[dep_feat].desc); + return 1; } deps &= ~(1ull << dep_feat); } if (!hyperv_feature_supported(cs, feature)) { if (hyperv_feat_enabled(cpu, feature)) { - fprintf(stderr, - "Hyper-V %s is not supported by kernel\n", - kvm_hyperv_properties[feature].desc); + error_setg(errp, "Hyper-V %s is not supported by kernel", + kvm_hyperv_properties[feature].desc); return 1; } else { return 0; @@ -1209,13 +1207,12 @@ static uint32_t hv_build_cpuid_leaf(CPUState *cs, uint32_t func, int reg) * of 'hv_passthrough' mode and fills the environment with all supported * Hyper-V features. */ -static int hyperv_expand_features(CPUState *cs) +static void hyperv_expand_features(CPUState *cs, Error **errp) { X86CPU *cpu = X86_CPU(cs); - int r; if (!hyperv_enabled(cpu)) - return 0; + return; if (cpu->hyperv_passthrough) { cpu->hyperv_vendor_id[0] = @@ -1262,37 +1259,60 @@ static int hyperv_expand_features(CPUState *cs) } /* Features */ - r = hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI); - r |= hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT); + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RELAXED, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VAPIC, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TIME, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_CRASH, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RESET, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_VPINDEX, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_RUNTIME, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_SYNIC, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_FREQUENCIES, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_REENLIGHTENMENT, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_TLBFLUSH, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_EVMCS, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_IPI, errp)) { + return; + } + if (hv_cpuid_check_and_set(cs, HYPERV_FEAT_STIMER_DIRECT, errp)) { + return; + } /* Additional dependencies not covered by kvm_hyperv_properties[] */ if (hyperv_feat_enabled(cpu, HYPERV_FEAT_SYNIC) && !cpu->hyperv_synic_kvm_only && !hyperv_feat_enabled(cpu, HYPERV_FEAT_VPINDEX)) { - fprintf(stderr, "Hyper-V %s requires Hyper-V %s\n", - kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, - kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); - r |= 1; - } - - if (r) { - return -ENOSYS; + error_setg(errp, "Hyper-V %s requires Hyper-V %s", + kvm_hyperv_properties[HYPERV_FEAT_SYNIC].desc, + kvm_hyperv_properties[HYPERV_FEAT_VPINDEX].desc); } - - return 0; } /* @@ -1527,9 +1547,10 @@ int kvm_arch_init_vcpu(CPUState *cs) env->apic_bus_freq = KVM_APIC_BUS_FREQUENCY; /* Paravirtualization CPUIDs */ - r = hyperv_expand_features(cs); - if (r < 0) { - return r; + hyperv_expand_features(cs, &local_err); + if (local_err) { + error_report_err(local_err); + return -ENOSYS; } if (hyperv_enabled(cpu)) {
Use standard error_setg() mechanism in hyperv_expand_features(). Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- target/i386/kvm/kvm.c | 101 +++++++++++++++++++++++++----------------- 1 file changed, 61 insertions(+), 40 deletions(-)