apport-hackers team mailing list archive

[Brian Murray <brian@xxxxxxxxxx>]

One thing that might reduce code duplication in add_info() is calling attach_command_output() in a for loop. For example something like this:

stuff = { 
         'UbuntuReport': ['ubuntu-report', 'show'],
         'lspci-xx': ['lspci', '-x'],
         'dmidecode': ['dmidecode', ''] 
        }   
for name in stuff:
    attach_command_output(report, [stuff[name][0], stuff[name][1]], name)
    dot()

You could still include comments indicating the grouping (audio related) in the dictionary definition which would mostly keep the logical groupings you have.

Diff comments:

> === added file 'bin/oem-getlogs'
> --- bin/oem-getlogs	1970-01-01 00:00:00 +0000
> +++ bin/oem-getlogs	2019-12-11 02:28:06 +0000
> @@ -0,0 +1,217 @@
> +#! /usr/bin/python3
> +
> +from apport.hookutils import *
> +
> +from glob import glob
> +from io import BytesIO
> +import zipfile
> +from problem_report import CompressedValue
> +import os, sys, tempfile, shutil, re
> +import time, subprocess
> +
> +opt_debug = False
> +
> +# Apport helper routines
> +def debug(text):
> +    if opt_debug:
> +        print("%s\n" %(text))
> +
> +def attach_command_output(report, command_list, key):
> +    debug(" %s" %(' '.join(command_list)))

Is the leading space here (" %s") necessary?

> +    log = command_output(command_list) # from hookutils
> +    if not log or log[:5] == "Error":
> +        return
> +    report[key] = log
> +
> +def attach_pathglob_as_zip(report, pathglob, key, data_filter=None, type="b"):
> +    """Use zip file here because tarfile module in linux can't
> +       properly handle file size 0 with content in /sys directory like
> +       edid file. zipfile module works fine here. So we use it.
> +
> +       type:
> +            a: for ascii  type of data # TODO get case_id from argv, and put that into report filenameata
> +            b: for binary type of data
> +       """
> +    if data_filter is None:
> +        data_filter = lambda x: x
> +    filelist = []
> +    for pg in pathglob:
> +        for file in glob(pg):
> +            filelist.append(file)
> +
> +    zipf = BytesIO()
> +    with zipfile.ZipFile(zipf, mode='w', compression=zipfile.ZIP_DEFLATED) as zipobj:
> +        for f in filelist:
> +            if opt_debug:
> +                print(key, f)
> +            if not os.path.isfile(f):
> +                if opt_debug:
> +                    print(f, "is not a file")
> +                continue
> +            if type == "a":
> +                with open(f) as f_fd:
> +                    data = f_fd.read()
> +                    zipobj.writestr(f, data_filter(data))
> +            else:
> +                zipobj.write(f)
> +    cvalue = CompressedValue()
> +    cvalue.set_value(zipf.getbuffer())
> +    report[key + ".zip"] = cvalue
> +
> +def attach_nvidia_debug_logs(report, keep_locale=False):
> +    # check if nvidia-bug-report.sh exists
> +    nv_debug_command = 'nvidia-bug-report.sh'
> +
> +    if shutil.which(nv_debug_command) is None:
> +        if opt_debug:
> +            print(nv_debug_command, "does not exists in PATH.")

"does not exist"

> +        return
> +
> +    env = os.environ.copy()
> +    if not keep_locale:
> +        env['LC_MESSAGES'] = 'C'
> +
> +    # output result to temp directory
> +    nv_tempdir = tempfile.mkdtemp()
> +    nv_debug_file =  'nvidia-bug-report'
> +    nv_debug_fullfile = os.path.join(nv_tempdir, nv_debug_file)
> +    nv_debug_cmd = [nv_debug_command, '--output-file', nv_debug_fullfile]
> +    try:
> +        with open(os.devnull, 'w') as devnull:
> +            subprocess.run(nv_debug_cmd, env=env, stdout=devnull, stderr=devnull)
> +            nv_debug_fullfile_gz = nv_debug_fullfile + ".gz"
> +            attach_file_if_exists(report, nv_debug_fullfile_gz, 'nvidia-bug-report.gz')
> +            os.unlink(nv_debug_fullfile_gz)
> +    except OSError as e:
> +        err = 'Error: ' + str(e)
> +
> +    os.rmdir(nv_tempdir)
> +
> +def dot():
> +    print(".", end="", flush=True)
> +
> +def build_packages():
> +    # related packages
> +    packages = ['apt', 'grub2']
> +
> +    # display
> +    packages.append('xorg')
> +    packages.append('gnome-shell')
> +
> +    # audio
> +    packages.append('alsa-base')
> +
> +    # hotkey and hotplugs
> +    packages.append('udev')
> +
> +    # networking issues
> +    packages.append('network-manager')
> +
> +    return packages
> +
> +
> +def helper_url_credential_filter(string_with_urls):
> +    return re.sub(r"://\w+?:\w+?@", "://USER:SECRET@", string_with_urls)
> +
> +def add_info(report):
> +    # Check if the DCD file is exist in the installer.
> +    attach_command_output(report, ['ubuntu-report', 'show'], 'UbuntuReport'); dot()
> +    attach_file_if_exists(report, '/etc/buildstamp', 'BuildStamp'); dot()
> +    # Basic hardare information
> +    attach_hardware(report); dot()
> +    attach_alsa(report); dot()
> +    attach_wifi(report); dot()
> +    attach_command_output(report, ['lspci', '-xxxx'], 'lspci-xxxx'); dot()
> +    attach_command_output(report, ['lshw', '-json', '-numeric'], 'lshw.json'); dot()
> +    attach_command_output(report, ['dmidecode'], 'dmidecode'); dot()
> +    attach_command_output(report, ['acpidump'], 'acpidump'); dot()
> +    attach_command_output(report,
> +        ['fwupdmgr', 'get-devices', '--show-all-devices', '--no-unreported-check'],
> +        'fwupdmgr_get-devices'); dot()
> +    attach_command_output(report, ['boltctl', 'list'], 'boltctl-list'); dot()
> +    attach_command_output(report, ['mokutil', '--sb-state'], 'mokutil---sb-state'); dot()
> +    attach_command_output(report, ['tlp-stat'], 'tlp-stat'); dot()
> +
> +    # More audio related
> +    attach_command_output(report, ['pactl', 'list'], 'pactl-list'); dot()
> +    attach_command_output(report, ['aplay', '-l'], 'aplay-l'); dot()
> +    attach_command_output(report, ['aplay', '-L'], 'aplay-L'); dot()
> +    attach_command_output(report, ['arecord', '-l'], 'arecord-l'); dot()
> +    attach_command_output(report, ['arecord', '-L'], 'arecord-L'); dot()
> +    attach_pathglob_as_zip(report, ['/usr/share/alsa/ucm/*/*'], "ALSA-UCM"); dot()
> +
> +    # FIXME: should be included in xorg in the future
> +    attach_pathglob_as_zip(report, ['/sys/devices/*/*/drm/card?/*/edid'], "EDID"); dot()
> +    attach_command_output(report, ['glxinfo'], 'glxinfo'); dot()
> +    attach_command_output(report, ['xrandr'], 'xrandr'); dot()
> +    attach_command_output(report, ['xinput'], 'xinput'); dot()
> +
> +    # nvidia-bug-reports.sh
> +    attach_nvidia_debug_logs(report); dot()
> +
> +    # FIXME: should be included in thermald in the future
> +    attach_pathglob_as_zip(report, [
> +        "/etc/thermald/*",
> +        "/sys/devices/virtual/thermal/*",
> +        "/sys/class/thermal/*"
> +        ], "THERMALD"); dot()
> +
> +    # all kernel and system messages
> +    attach_pathglob_as_zip(report, ["/var/log/*", "/var/log/*/*"], "VAR_LOG"); dot()
> +
> +    # apt configs
> +    attach_pathglob_as_zip(report, [
> +        "/etc/apt/apt.conf.d/*",
> +        "/etc/apt/sources.list",
> +        "/etc/apt/sources.list.d/*.list",
> +        "/etc/apt/preferences.d/*"], "APT_CONFIGS",
> +        type="a", data_filter=helper_url_credential_filter); dot()
> +
> +    # TODO: debug information for suspend or hibernate
> +
> +    # packages installed.
> +    attach_command_output(report, ['dpkg', '-l'], 'dpkg-l'); dot()
> +
> +    # FIXME: should be included in bluez in the future
> +    attach_command_output(report, ['hciconfig', '-a'], 'hciconfig-a'); dot()
> +
> +    # FIXME: should be included in dkms in the future
> +    attach_command_output(report, ['dkms', 'status'], 'dkms_status'); dot()
> +
> +    # enable when the feature to include data from package hooks exists.
> +    # packages = build_packages()
> +    # attach_related_packages(report, packages)
> +
> +if __name__ == '__main__':
> +    from argparse import ArgumentParser
> +    import gzip
> +
> +    parser = ArgumentParser(prog="oem-getlogs",
> +        usage="Useage: sudo oem-getlogs [-c CASE_ID]",
> +        description="Get Hardware Enablement related logs")
> +    parser.add_argument("-c", "--case-id", help="optional CASE_ID", dest="cid", default="")
> +    args = parser.parse_args()
> +
> +    # check if we got root permission
> +    if os.geteuid() != 0:
> +        print("Error: you need to run this program as root")
> +        parser.print_help()
> +        sys.exit(1)
> +
> +    print("Start to collect logs: ", end="", flush=True)
> +    # create report
> +    report = apport.Report()
> +    add_info(report)
> +
> +    # generate filename
> +    hostname = os.uname()[1]
> +    date_time = time.strftime("%Y%m%d%H%M%S%z", time.localtime())
> +    filename_lst = ["oemlogs", hostname]
> +    if (len(args.cid) > 0):
> +        filename_lst.append(args.cid)
> +    filename_lst.append(date_time + ".apport.gz")
> +    filename = "-".join(filename_lst)
> +
> +    with gzip.open(filename, 'wb') as f:
> +        report.write(f)
> +    print("\nSaved log to", filename)


-- 
https://code.launchpad.net/~ycheng-twn/apport/bug-1841157/+merge/373802
Your team Apport upstream developers is requested to review the proposed merge of lp:~ycheng-twn/apport/bug-1841157 into lp:apport.