From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751097AbdEAXoT (ORCPT ); Mon, 1 May 2017 19:44:19 -0400 Received: from mail-io0-f176.google.com ([209.85.223.176]:34163 "EHLO mail-io0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750743AbdEAXoR (ORCPT ); Mon, 1 May 2017 19:44:17 -0400 MIME-Version: 1.0 In-Reply-To: <20170429053720.GB11491@kroah.com> References: <20170428204225.32043-1-jwerner@chromium.org> <20170428204225.32043-3-jwerner@chromium.org> <20170429053720.GB11491@kroah.com> From: Julius Werner Date: Mon, 1 May 2017 16:44:15 -0700 X-Google-Sender-Auth: 613cjuoHPlFyAwfzS-iKpf9lxEY Message-ID: Subject: Re: [PATCH v2 2/3] firmware: google: memconsole: Escape unprintable characters To: Greg Kroah-Hartman Cc: Julius Werner , LKML , Thierry Escande , Dmitry Torokhov , Aaron Durbin Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 28, 2017 at 10:37 PM, Greg Kroah-Hartman wrote: > On Fri, Apr 28, 2017 at 01:42:24PM -0700, Julius Werner wrote: >> Recent improvements in coreboot's memory console allow it to contain >> logs from more than one boot as long as the information persists in >> memory. Since trying to persist a memory buffer across reboots often >> doesn't quite work perfectly it is now more likely for random bit flips >> to occur in the console. With the current implementation this can lead >> to stray control characters that cause weird effects for the most common >> use cases (such as just dumping the console with 'cat'). >> >> This patch changes the memconsole driver to replace unprintable >> characters with '?' by default. It also adds a new /sys/firmware/rawlog >> node next to the existing /sys/firmware/log for use cases where it's >> desired to read the raw characters. > > Again, you are doing multiple things here. Break it up. Sorry, I'm not sure what else you want me to break up here? If I add the escaping in one patch and then add the raw node in a different patch, there's a gap between the two patches where we're losing functionality. Isn't that undesirable? > And, can userspace handle this change? You are now changing the format > of the existing file. I'm escaping characters that previously hadn't been escaped, but really, I'm just trying to make sure that people can continue to use this node as before. The real change has already happened in coreboot, outside of Linux. This node could previously be relied upon to only contain printable characters, and with newer versions of coreboot that's not always the case anymore. I chose to do it this way because I thought it would be the least disruptive for most users. It can be handled in userspace, yes. It depends on what you use to read the file, mostly... people using less will be fine, but people using cat may get weird behavior. You're right that it doesn't really *need* to be escaped in the kernel, and honestly I don't care too much... I can leave it out if you want. But I think this is the most convenient way to do it for existing users. >> - return sysfs_create_bin_file(firmware_kobj, &memconsole_bin_attr); >> + return sysfs_create_bin_file(firmware_kobj, &memconsole_log_attr) || >> + sysfs_create_bin_file(firmware_kobj, &memconsole_raw_attr); > > While it is really rare, please do this properly and create one file, > and then the other, and provide proper error handling here (i.e. if the > second fails, remove the first). Okay, will send that with the next set once we agree on the other stuff.