On 13/01/22 6:47 pm, Ani Sinha wrote:


On Thu, Jan 13, 2022 at 4:50 PM John Levon <levon@movementarian.org> 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.

Thankyou Ani ! Let me know if I need to update anything else for the patch.

Thanks :)