Re: [PATCH v5 5/9] pvs: get info about serial ports

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

 



On 04/20/2012 10:01 AM, Dmitry Guryanov wrote:
> Add support of collecting information about serial
> ports. This change is needed mostly as an example,
> support of other devices will be added later.
> 
> Signed-off-by: Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>
> ---
>  src/pvs/pvs_driver.c |  115 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 115 insertions(+), 0 deletions(-)

This is as far as I got today; at this point, it's probably okay for you
to start respinning your patches (some of my comments have been pretty
common between the patches).


>  
> +static int
> +pvsGetSerialInfo(virDomainChrDefPtr chr,
> +                 const char *name, virJSONValuePtr value)
> +{
> +    const char *tmp;
> +
> +    chr->deviceType = VIR_DOMAIN_CHR_DEVICE_TYPE_SERIAL;
> +    chr->targetType = VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL;
> +    chr->target.port = atoi(name + strlen("serial"));

atoi() is a no-no, because it fails to detect errors (but we don't yet
enable the syntax-check rule for it; I should really fix that some day).
 virStrToLong_i() is your friend.

Overall, this series is looking relatively decent (it's compiling and
mostly passing syntax checks, other than the po/POTFILES.in issues).
I'm not really in a position to test it, but I'm okay accepting it for
0.9.12, since as new code it can't really be causing regressions.  I'm
not sure if it will be in before DV cuts rc1, but I think it's probably
okay even if we miss the first release candidate build before you can
get my review comments incorporated.

-- 
Eric Blake   eblake@xxxxxxxxxx    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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