On Thu, Jan 13, 2022 at 03:11:51PM +0530, Ani Sinha wrote:
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index d822533ccb..4130df0ed9 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -10718,6 +10718,8 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
> > g_autoptr(virJSONValue) props = NULL;
> > g_autofree char *chardev = g_strdup_printf("char%s", serial->info.alias);
> > virQEMUCapsFlags caps;
> > + const char *typestr;
> > + int ret;
>
> type should match the return type of this function.
It does match return type of virDomainChrSerialTargetModelTypeToString():
47 #define VIR_ENUM_DECL(name) \
48 const char *name ## TypeToString(int type); \
> ret should be defined as virJSONValue.
"int" is correct:
358 int
359 virJSONValueObjectAdd(virJSONValue **objptr, ...)
> I preferred your previous style to this one.
Below is cleaner IMO: we don't repeat code, and the flow is much clearer.
> > switch ((virDomainChrSerialTargetModel) serial->targetModel) {
> > case VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL:
> > @@ -10750,11 +10752,24 @@ qemuBuildSerialChrDeviceProps(const virDomainDef *def,
> > return NULL;
> > }
> >
> > - if (virJSONValueObjectAdd(&props,
> > - "s:driver", virDomainChrSerialTargetModelTypeToString(serial->targetModel),
> > - "s:chardev", chardev,
> > - "s:id", serial->info.alias,
> > - NULL) < 0)
> > + typestr = virDomainChrSerialTargetModelTypeToString(serial->targetModel);
> > +
> > + if (serial->targetModel == VIR_DOMAIN_CHR_SERIAL_TARGET_MODEL_ISA_SERIAL) {
> > + ret = virJSONValueObjectAdd(&props,
> > + "s:driver", typestr,
> > + "s:chardev", chardev,
> > + "s:id", serial->info.alias,
> > + "k:index", serial->target.port,
> > + NULL);
> > + } else {
> > + ret = virJSONValueObjectAdd(&props,
> > + "s:driver", typestr,
> > + "s:chardev", chardev,
> > + "s:id", serial->info.alias,
> > + NULL);
> > + }
> > +
> > + if (ret < 0)
> > return NULL;
regards
john
