From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:53127)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UD1CM-00020k-DV
	for qemu-devel@nongnu.org; Tue, 05 Mar 2013 18:23:08 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UD1CJ-0002Md-Mv
	for qemu-devel@nongnu.org; Tue, 05 Mar 2013 18:23:06 -0500
Received: from mx1.redhat.com ([209.132.183.28]:15527)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <lersek@redhat.com>) id 1UD1CJ-0002MZ-Da
	for qemu-devel@nongnu.org; Tue, 05 Mar 2013 18:23:03 -0500
Message-ID: <51367E77.30404@redhat.com>
Date: Wed, 06 Mar 2013 00:23:35 +0100
From: Laszlo Ersek <lersek@redhat.com>
MIME-Version: 1.0
References: <1362435597-20018-1-git-send-email-lersek@redhat.com>
	<1362435597-20018-4-git-send-email-lersek@redhat.com>
	<51366164.8020806@redhat.com>
In-Reply-To: <51366164.8020806@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus()
 for Linux with sysfs
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org, pbonzini@redhat.com, mdroth@linux.vnet.ibm.com, lcapitulino@redhat.com

On 03/05/13 22:19, Eric Blake wrote:
> On 03/04/2013 03:19 PM, Laszlo Ersek wrote:

>> +            } else {
>> +                unsigned online;
>> +
>> +                if (fscanf(f, "%u", &online) != 1) {
>> +                    error_setg(&local_err, "failed to read or parse \"%s\"",
>> +                               buf);
> 
> Does doing a scan of the file's existing contents buy us any safety?
> Why not just blindly write into the file, instead of first reading it?

:)

For an already online CPU:

# dd of=/sys/devices/system/cpu/cpu1/online bs=1 count=1 <<<1
dd: writing `/sys/devices/system/cpu/cpu1/online': Invalid argument
[...]

In the kernel,

store_online() [drivers/base/cpu.c]
  cpu_up() [kernel/cpu.c]
    _cpu_up()

	if (cpu_online(cpu) || !cpu_present(cpu)) {
		ret = -EINVAL;
		goto out;
	}

This logic seems to have been present since the origin of the current
Linux repo (1da177e4 "Linux-2.6.12-rc2").

Checking the history tree at
<git://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git>, the
logic dates back to the very first committed version of cpu_up():

commit c5e062079a7090891ea5cd1b23a7eab52b156b2a
Author: Rusty Russell <rusty@rustcorp.com.au>
Date:   Fri Jul 26 01:28:07 2002 -0700

    [PATCH] Hot-plug CPU Boot Changes
...

> 
>> +                } else if ((online != 0) != vcpu->online) {
>> +                    errno = 0;
>> +                    rewind(f);
>> +                    if (errno != 0 ||
>> +                        fprintf(f, "%u\n", (unsigned)vcpu->online) < 0) {
> 
> Do you really want to be printing NUL or \1?  I though the kernel
> interface expected the literal character '0' or '1' (in ascii, \x30 or
> \x31).

I'm using the %u conversion specifier, which turns the unsigned int
argument into decimal string representation. Same as %d for signed int.

Thanks for all review comments!
Laszlo