Re: [PATCH 2/2 for 8.0] qemu: add index for isa-serial device using target.port

On Thu, Jan 13, 2022 at 4:50 PM John Levon <levon(a)movementarian.org&gt; wrote: 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); \ Yes you are correct. I was thinking of qemuBuildSerialChrDeviceProps() but it does not return ret directly from this function. Another reason it makes me uncomfortable. > I preferred your previous style to this one. Below is cleaner IMO: we don't repeat code, and the flow is much clearer. If you look at other functions for example qemuBuildVirtioDevProps() etc they follow the previous style. I think it's fine to return NULL from multiple points. In any case if the maintainers are fine with it , I'm ok. Style is trivial.