| Message ID | 20191206213433.11305-3-wainersm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | python/qemu: New accel module and improvements | expand |
On Fri, Dec 06, 2019 at 04:34:31PM -0500, Wainer dos Santos Moschetta wrote: > Since commit cbe6d6365a48 the command `qemu -accel help` returns > the list of accelerators enabled in the QEMU binary. This adds > the list_accel() method which return that same list. > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > python/qemu/accel.py | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/python/qemu/accel.py b/python/qemu/accel.py > index cbeac10dd1..746b7e68f5 100644 > --- a/python/qemu/accel.py > +++ b/python/qemu/accel.py > @@ -14,7 +14,11 @@ accelerators. > # the COPYING file in the top-level directory. > # > > +import logging > import os > +import subprocess > + > +LOG = logging.getLogger(__name__) > > # Mapping host architecture to any additional architectures it can > # support which often includes its 32 bit cousin. > @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = { > "aarch64" : "armhf" > } > > +def list_accel(qemu_bin): > + """ > + List accelerators enabled in the QEMU binary. > + > + @param qemu_bin (str): path to the QEMU binary. > + @raise Exception: if failed to run `qemu -accel help` > + @return a list of accelerator names. > + """ > + if not qemu_bin: > + return [] > + try: > + out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True) There's no need to use a shell here. This could become: out = subprocess.check_output([qemu_bin, '-accel' 'help']) > + except: > + LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin) > + raise > + lines = out.decode().splitlines() And maybe discard the first line earlier with: lines = out.decode().splitlines()[1:] Also, you could avoid the manual decode() with the `universal_newlines` option to subprocess.check_output(), ie: accels = subprocess.check_output([qemu-bin, '-accel', 'help'], universal_newlines=True).splitlines()[1:] > + # Skip the first line which is the header. > + return [l.strip() for l in lines[1:] if l] > + I think that the `if l` check can actually hide undesirable behavior (bugs) in the `qemu -accel ?` output. I don't remember seeing `-$(option) ?` returning empty strings but doesn't mean it couldn't and shouldn't). I do remember `-machine ?` returning random non-printable characters that turned out to be a bug, though. > def kvm_available(target_arch=None): > host_arch = os.uname()[4] > if target_arch and target_arch != host_arch: > -- > 2.21.0 > - Cleber.
On 12/9/19 10:52 PM, Cleber Rosa wrote: > On Fri, Dec 06, 2019 at 04:34:31PM -0500, Wainer dos Santos Moschetta wrote: >> Since commit cbe6d6365a48 the command `qemu -accel help` returns >> the list of accelerators enabled in the QEMU binary. This adds >> the list_accel() method which return that same list. >> >> Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> >> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> python/qemu/accel.py | 23 +++++++++++++++++++++++ >> 1 file changed, 23 insertions(+) >> >> diff --git a/python/qemu/accel.py b/python/qemu/accel.py >> index cbeac10dd1..746b7e68f5 100644 >> --- a/python/qemu/accel.py >> +++ b/python/qemu/accel.py >> @@ -14,7 +14,11 @@ accelerators. >> # the COPYING file in the top-level directory. >> # >> >> +import logging >> import os >> +import subprocess >> + >> +LOG = logging.getLogger(__name__) >> >> # Mapping host architecture to any additional architectures it can >> # support which often includes its 32 bit cousin. >> @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = { >> "aarch64" : "armhf" >> } >> >> +def list_accel(qemu_bin): >> + """ >> + List accelerators enabled in the QEMU binary. >> + >> + @param qemu_bin (str): path to the QEMU binary. >> + @raise Exception: if failed to run `qemu -accel help` >> + @return a list of accelerator names. >> + """ >> + if not qemu_bin: >> + return [] >> + try: >> + out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True) > There's no need to use a shell here. This could become: > > out = subprocess.check_output([qemu_bin, '-accel' 'help']) Ack > >> + except: >> + LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin) >> + raise >> + lines = out.decode().splitlines() > And maybe discard the first line earlier with: > > lines = out.decode().splitlines()[1:] > > Also, you could avoid the manual decode() with the `universal_newlines` > option to subprocess.check_output(), ie: > > accels = subprocess.check_output([qemu-bin, '-accel', 'help'], > universal_newlines=True).splitlines()[1:] Nice. v3 will have universal_newlines=True. > >> + # Skip the first line which is the header. >> + return [l.strip() for l in lines[1:] if l] >> + > I think that the `if l` check can actually hide undesirable behavior > (bugs) in the `qemu -accel ?` output. I don't remember seeing > `-$(option) ?` returning empty strings but doesn't mean it couldn't > and shouldn't). > > I do remember `-machine ?` returning random non-printable characters > that turned out to be a bug, though. Double-checking: are you suggesting to remove the 'if not empty' check so that bugs on output could emerge? Thanks! - Wainer > >> def kvm_available(target_arch=None): >> host_arch = os.uname()[4] >> if target_arch and target_arch != host_arch: >> -- >> 2.21.0 >> > - Cleber.
On Wed, Dec 11, 2019 at 02:58:35PM -0200, Wainer dos Santos Moschetta wrote: > > On 12/9/19 10:52 PM, Cleber Rosa wrote: > > On Fri, Dec 06, 2019 at 04:34:31PM -0500, Wainer dos Santos Moschetta wrote: > > > Since commit cbe6d6365a48 the command `qemu -accel help` returns > > > the list of accelerators enabled in the QEMU binary. This adds > > > the list_accel() method which return that same list. > > > > > > Signed-off-by: Wainer dos Santos Moschetta <wainersm@redhat.com> > > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > > > --- > > > python/qemu/accel.py | 23 +++++++++++++++++++++++ > > > 1 file changed, 23 insertions(+) > > > > > > diff --git a/python/qemu/accel.py b/python/qemu/accel.py > > > index cbeac10dd1..746b7e68f5 100644 > > > --- a/python/qemu/accel.py > > > +++ b/python/qemu/accel.py > > > @@ -14,7 +14,11 @@ accelerators. > > > # the COPYING file in the top-level directory. > > > # > > > +import logging > > > import os > > > +import subprocess > > > + > > > +LOG = logging.getLogger(__name__) > > > # Mapping host architecture to any additional architectures it can > > > # support which often includes its 32 bit cousin. > > > @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = { > > > "aarch64" : "armhf" > > > } > > > +def list_accel(qemu_bin): > > > + """ > > > + List accelerators enabled in the QEMU binary. > > > + > > > + @param qemu_bin (str): path to the QEMU binary. > > > + @raise Exception: if failed to run `qemu -accel help` > > > + @return a list of accelerator names. > > > + """ > > > + if not qemu_bin: > > > + return [] > > > + try: > > > + out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True) > > There's no need to use a shell here. This could become: > > > > out = subprocess.check_output([qemu_bin, '-accel' 'help']) > > Ack > > > > > > + except: > > > + LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin) > > > + raise > > > + lines = out.decode().splitlines() > > And maybe discard the first line earlier with: > > > > lines = out.decode().splitlines()[1:] > > > > Also, you could avoid the manual decode() with the `universal_newlines` > > option to subprocess.check_output(), ie: > > > > accels = subprocess.check_output([qemu-bin, '-accel', 'help'], > > universal_newlines=True).splitlines()[1:] > > Nice. v3 will have universal_newlines=True. > > > > > > + # Skip the first line which is the header. > > > + return [l.strip() for l in lines[1:] if l] > > > + > > I think that the `if l` check can actually hide undesirable behavior > > (bugs) in the `qemu -accel ?` output. I don't remember seeing > > `-$(option) ?` returning empty strings but doesn't mean it couldn't > > and shouldn't). > > > > I do remember `-machine ?` returning random non-printable characters > > that turned out to be a bug, though. > > Double-checking: are you suggesting to remove the 'if not empty' check so > that bugs on output could emerge? > Yes, that's my suggestion. I don't think we need to process QEMU's output beyond what it's expected to be returned. Cheers, - Cleber. > Thanks! > > - Wainer > > > > > > def kvm_available(target_arch=None): > > > host_arch = os.uname()[4] > > > if target_arch and target_arch != host_arch: > > > -- > > > 2.21.0 > > > > > - Cleber. > >
diff --git a/python/qemu/accel.py b/python/qemu/accel.py index cbeac10dd1..746b7e68f5 100644 --- a/python/qemu/accel.py +++ b/python/qemu/accel.py @@ -14,7 +14,11 @@ accelerators. # the COPYING file in the top-level directory. # +import logging import os +import subprocess + +LOG = logging.getLogger(__name__) # Mapping host architecture to any additional architectures it can # support which often includes its 32 bit cousin. @@ -23,6 +27,25 @@ ADDITIONAL_ARCHES = { "aarch64" : "armhf" } +def list_accel(qemu_bin): + """ + List accelerators enabled in the QEMU binary. + + @param qemu_bin (str): path to the QEMU binary. + @raise Exception: if failed to run `qemu -accel help` + @return a list of accelerator names. + """ + if not qemu_bin: + return [] + try: + out = subprocess.check_output("%s -accel help" % qemu_bin, shell=True) + except: + LOG.debug("Failed to get the list of accelerators in %s" % qemu_bin) + raise + lines = out.decode().splitlines() + # Skip the first line which is the header. + return [l.strip() for l in lines[1:] if l] + def kvm_available(target_arch=None): host_arch = os.uname()[4] if target_arch and target_arch != host_arch: