Re: [Qemu-devel] [PATCH 11/18] scripts/qemu.py: support adding a console with the default serial device

[Cleber Rosa <crosa@redhat.com>]

On 1/31/19 1:49 PM, Wainer dos Santos Moschetta wrote:
> Hi Cleber,
> 
> me again. :)
> 
> On 01/17/2019 04:56 PM, Cleber Rosa wrote:
>> The set_console() utility function traditionally adds a device either
>> based on the explicitly given device type, or based on the machine type,
>> a known good type of device.
>>
>> But, for a number of machine types, it may be impossible or
>> inconvenient to add the devices my means of "-device" command line
>> options, and then it may better to just use the "-serial" option and
>> let QEMU itself, based on the machine type, set the device
>> accordingly.
>>
>> To achieve that, the behavior of set_console() now flags the intention
>> to add a console device on launch(), and if no explicit device type is
>> given, and there's no definition on CONSOLE_DEV_TYPES, the "-serial"
>> is going to be added to the QEMU command line, instead of raising
>> exceptions.
>>
>> Signed-off-by: Cleber Rosa <crosa@redhat.com>
>> ---
>>   scripts/qemu.py | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/scripts/qemu.py b/scripts/qemu.py
>> index ec3567d4e2..88e1608b42 100644
>> --- a/scripts/qemu.py
>> +++ b/scripts/qemu.py
>> @@ -121,6 +121,7 @@ class QEMUMachine(object):
>>           self._temp_dir = None
>>           self._launched = False
>>           self._machine = None
>> +        self._console_set = False
>>           self._console_device_type = None
>>           self._console_address = None
>>           self._console_socket = None
>> @@ -240,13 +241,17 @@ class QEMUMachine(object):
>>                   '-display', 'none', '-vga', 'none']
>>           if self._machine is not None:
>>               args.extend(['-machine', self._machine])
>> -        if self._console_device_type is not None:
>> +        if self._console_set:
>>               self._console_address = os.path.join(self._temp_dir,
>>                                                    self._name +
>> "-console.sock")
>>               chardev = ('socket,id=console,path=%s,server,nowait' %
>>                          self._console_address)
>> -            device = '%s,chardev=console' % self._console_device_type
>> -            args.extend(['-chardev', chardev, '-device', device])
>> +            args.extend(['-chardev', chardev])
>> +            if self._console_device_type is None:
>> +                args.extend(['-serial', 'chardev:console'])
>> +            else:
>> +                device = '%s,chardev=console' %
>> self._console_device_type
>> +                args.extend(['-device', device])
>>           return args
>>         def _pre_launch(self):
>> @@ -479,23 +484,20 @@ class QEMUMachine(object):
>>           machine launch time, as it depends on the temporary directory
>>           to be created.
>>   -        @param device_type: the device type, such as "isa-serial"
>> +        @param device_type: the device type, such as "isa-serial".  If
>> +                            None is given (the default value) a "-serial
>> +                            chardev:console" command line argument will
>> +                            be used instead, resorting to the machine's
>> +                            default device type.
> 
> Shouldn't you mention it will look for device type on CONSOLE_DEV_TYPES
> if device_type is None and machine is not None?
> 

Yes, you're right.  How about this:

...
        @param device_type: the device type, such as "isa-serial".  If
                            None is given (the default value) a "-serial
                            chardev:console" command line argument will
                            be used instead, resorting to the machine's
                            default device type, if a machine type is set,
                            and a matching entry exists on
CONSOLE_DEV_TYPES.

...

> The description on set_console()'s docstring is out-of-sync with current
> implementation too.
> 

Good catch!  I'm rewriting that as:

...

        This is a convenience method that will either use the provided
        device type, of if not given, it will use the device type set
        on CONSOLE_DEV_TYPES if a machine type is set, and a matching
        entry exists on CONSOLE_DEV_TYPES.

...

How does it sound?

Thanks!
- Cleber.

> - Wainer
> 
>>           @raises: QEMUMachineAddDeviceError if the device type is not
>> given
>>                    and can not be determined.
>>           """
>> -        if device_type is None:
>> -            if self._machine is None:
>> -                raise QEMUMachineAddDeviceError("Can not add a
>> console device:"
>> -                                                " QEMU instance
>> without a "
>> -                                                "defined machine type")
>> +        self._console_set = True
>> +        if device_type is None and self._machine is not None:
>>               for regex, device in CONSOLE_DEV_TYPES.items():
>>                   if re.match(regex, self._machine):
>>                       device_type = device
>>                       break
>> -            if device_type is None:
>> -                raise QEMUMachineAddDeviceError("Can not add a
>> console device:"
>> -                                                " no matching console
>> device "
>> -                                                "type definition")
>>           self._console_device_type = device_type
>>         @property
> 
>