[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 20/21] conf: convert sclp/sclplm <console> as <serial>



On Tue, 2017-11-21 at 17:42 +0100, Andrea Bolognani wrote:
> +static void
> +virDomainChrConsoleTargetTypeToSerial(virDomainChrConsoleTargetType type,
> +                                      int *retType, int *retModel)

The last two arguments should be each on a separate line.
I'd also prefer the names 'targetType' and 'targetModel'.

> @@ -4014,7 +4048,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
>      }
>      if (def->nconsoles > 0 && def->os.type == VIR_DOMAIN_OSTYPE_HVM &&
>          (def->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL ||
> -         def->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE)) {
> +         def->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE ||
> +         (def->consoles[0]->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLP &&
> +          (parseFlags & VIR_DOMAIN_DEF_PARSE_ABI_UPDATE)))) {

VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SCLPLM should be included in the
check above, or <console/>s with <target type='sclplm'/> will not
be converted into <serial/>s.

> @@ -4027,6 +4063,8 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
>  
>          /* create the serial port definition from the console definition */
>          if (def->nserials == 0) {
> +            virDomainChrConsoleTargetType type = def->consoles[0]->targetType;
> +
>              if (VIR_APPEND_ELEMENT(def->serials,
>                                     def->nserials,
>                                     def->consoles[0]) < 0)
> @@ -4034,7 +4072,9 @@ virDomainDefAddConsoleCompat(virDomainDefPtr def,
>  
>              /* modify it to be a serial port */
>              def->serials[0]->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
> -            def->serials[0]->targetType = VIR_DOMAIN_CHR_SERIAL_TARGET_TYPE_NONE;
> +            virDomainChrConsoleTargetTypeToSerial(type,
> +                                                  &def->serials[0]->targetType,
> +                                                  &def->serials[0]->targetModel);

Drop 'type' and just use 'def->consoles[0]->targetType' here.


Everything else looks good, so my R-b stands. I can fix all of the
above before pushing (assuming a respin won't be necessary).

-- 
Andrea Bolognani / Red Hat / Virtualization


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]