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

Re: [libvirt] [PATCH 2/3] PHYP: Adding basic network functions



On 11/19/2010 07:55 AM, Eduardo Otubo wrote:
> Now adding some basic operation network functions and its UUID
> "helpers".
> ---
>  src/phyp/phyp_driver.c |  202 ++++++++++++++++++++++++++++++++++++++++++++++-
>  src/phyp/phyp_uuid.c   |  177 ++++++++++++++++++++++++++++++++++++++++++
>  src/phyp/phyp_uuid.h   |    8 ++
>  3 files changed, 382 insertions(+), 5 deletions(-)

>  
> +int
> +phypListNetworkMAC(virConnectPtr conn, long long *macs, int nnets)

This should probably be static.

> +{
> +    ConnectionData *connection_data = conn->networkPrivateData;
> +    phyp_driverPtr phyp_driver = conn->privateData;
> +    LIBSSH2_SESSION *session = connection_data->session;
> +    int system_type = phyp_driver->system_type;
> +    char *managed_system = phyp_driver->managed_system;
> +    int vios_id = phyp_driver->vios_id;
> +    int exit_status = 0;
> +    int got = -1;
> +    char *cmd = NULL;
> +    char *ret = NULL;
> +    char *line, *next_line;
> +    virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> +    virBufferAddLit(&buf, "lshwres");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, " -m %s", managed_system);
> +
> +    virBufferVSprintf(&buf, " -r virtualio --rsubtype eth --level lpar"
> +                      "|grep -v lpar_id=%d| sed -e 's/^.*mac_addr=//g'",

That two-process grep | sed is more efficient as one, and ^ can only
match once, so the g is pointless:

| sed '/lpar_id=%d/d; s/^.*mac_addr=//'

> +int
> +phypListNetworks(virConnectPtr conn, char **const names, int nnames)

Likewise a candidate for static (in fact, it's sometimes a good idea to
mark everything new as static, see what doesn't compile, and fix the few
fallouts - it's always better to shoot for the smallest possible scope).

> +    virBufferAddLit(&buf, "lshwres");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, " -m %s", managed_system);
> +    virBufferVSprintf(&buf, " -r virtualio --rsubtype slot  --level slot"
> +                      "|grep eth|grep -v lpar_id=%d|"
> +                      "sed -e 's/^.*drc_name=//g'", vios_id);

Here, you can replace grep | grep | sed with:

sed -n '/lpar_id=%d/d; /eth/ s/^.*drc_name=//p'

Mostly looks okay, but there's still the issue of using long long
instead of a 6-byte array for MACs that might be worth changing.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


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