apport-hackers team mailing list archive

[Brian Murray <brian@xxxxxxxxxx>]

I apologize for the delay in reviewing this. I approve of the new direction but there are some changes I'd like to see made and my comments can be found inline.

Diff comments:

> === added file 'bin/oem-getlogs'
> --- bin/oem-getlogs	1970-01-01 00:00:00 +0000
> +++ bin/oem-getlogs	2019-10-08 08:31:41 +0000
> @@ -0,0 +1,208 @@
> +#! /usr/bin/python3
> +
> +from apport.hookutils import *
> +
> +from glob import glob
> +from io import BytesIO
> +import zipfile
> +from problem_report import CompressedValue
> +import os, tempfile
> +import time
> +
> +opt_debug = False
> +
> +# Apport helper routines
> +def debug(text):
> +    if opt_debug:
> +        sys.stderr.write("%s\n" %(text))

sys.stdout makes more sense for an informative message.

> +
> +def attach_command_output(report, command_list, key):
> +    debug(" %s" %(' '.join(command_list)))
> +    log = command_output(command_list)
> +    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 contaent in /sys directory like

Typo of content

> +       edid file. zipfile modulorks fine here. So we use it.

Maybe it should be "zipfile module works fine".

> +
> +       type:
> +            a: for ascii type o    # TODO get case_id from argv, and put that into report filenameata

Is there something missing after "type o"?

> +            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()
> +    zipobj = zipfile.ZipFile(zipf, mode='w')

I think using 'with zipfile.ZipFile(zipf, mode='w') as zipobj:' would better and avoid the need for zipobj.close() in line 58.

> +    for f in filelist:
> +        if opt_debug:
> +            print(key, f)
> +        if not os.path.isfile(f):
> +            continue

It'd probably make sense to provide debugging information if os.path.isfile(f) is not true.

> +        if type == "a":
> +            with open(f) as f_fd:
> +                data = f_fd.read()
> +                zipobj.writestr(f, data_filter(data))
> +        else:
> +            zipobj.write(f)
> +    zipobj.close()
> +    cvalue = CompressedValue()
> +    cvalue.set_value(zipf.getbuffer())
> +    report[key + ".zip"] = cvalue
> +
> +def attach_nvidia_debug_logs(report, keep_locale=False):
> +    env = os.environ.copy()
> +    if not keep_locale:
> +        env['LC_MESSAGES'] = 'C'
> +
> +    nv_tempdir = tempfile.mkdtemp()
> +    nv_debug_file =  'nvidia-bug-report'
> +    nv_debug_fullfile = os.path.join(nv_tempdir, nv_debug_file)
> +    nv_debug_cmd = ['nvidia-bug-report.sh', '--output-file', nv_debug_fullfile]

You should ensure that 'nvidia-bug-report.sh' is available before calling it.

> +    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("://\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', '-x'], 'lspci-xx'); dot()

Only one x is passed to lspci but the file name has two x's i.e. 'lspci-xx'.

> +    attach_command_output(report, ['lshw', '-json'], '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: shold be inclued in thermald in the future

"should be included"

> +    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: shold be inclued in bluez in the future

"should be included"

> +    attach_command_output(report, ['hciconfig'], 'hciconfig'); 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()
> +        os._exit(-1)

Please change this to 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]
> +    now = time.localtime()
> +    date_time = time.strftime("%Y%m%d%H%M%S%z", now)

Storing time.localtime() as a variable now when you are only using it once is unnecessary. This would be simpler as "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)

You might consider changing permissions on the file (depending on how sensitive the information in it is) so that the user doesn't need to keep being root to move it around.

> +    print("\nSave log to", filename)

Change to "Saved log to" so we use the past tense.



-- 
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.