Re: [libvirt] [PATCH v2] Implement CPU topology support for QEMU driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 15, 2010 at 01:31:49PM +0100, Jiri Denemark wrote:
> QEMU's command line equivalent for the following domain XML fragment
>     <vcpus>2</vcpus>
>     <cpu ...>
>         ...
>         <topology sockets='1' cores='2', threads='1'/>
>     </cpu>
> 
> is
> 
>     -smp 2,sockets=1,cores=2,threads=1
> 
> This syntax was introduced in QEMU-0.12.
> 
> Version 2 changes:
> - -smp argument build split into a separate function
> - always add ",sockets=S,cores=C,threads=T" to -smp if qemu supports it
> - use qemuParseCommandLineKeywords for command line parsing
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>


> @@ -2112,8 +2148,13 @@ int qemudBuildCommandLine(virConnectPtr conn,
>          ADD_ARG_LIT("-mem-path");
>          ADD_ARG_LIT(driver->hugepage_path);
>      }
> +
> +    if (!(smp = qemudBuildCommandLineSmp(conn, def, qemuCmdFlags)))
> +        goto error;
> +
>      ADD_ARG_LIT("-smp");
> -    ADD_ARG_LIT(vcpus);
> +    ADD_ARG_LIT(smp);
> +    VIR_FREE(smp);


If you've got an allocated string, then just use 'ADD_ARG(smp)' and which
avoids the strdup() that ADD_ARG_LIT does and avoids need for VIR_FREE
too. Also you should move the qemudBuildCommandLineSmp() call to *after*
the ADD_ARG_LIT("-smp") line, otherwise you can leak 'smp' on OOM handling
in the ADD_ARG_LIT("-smp") call.


ACK, if you make that minor memory handling fix before committing

Regards,
Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]