Re: [Qemu-devel] [PATCH 3/3] qga: implement qmp_guest_set_vcpus() for Linux with sysfs

[Laszlo Ersek]

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 <address@hidden>
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