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

Re: [libvirt] [PATCH 3/3] PHYP: create, destroy and other network functions



On 11/19/2010 07:55 AM, Eduardo Otubo wrote:
> Adding networkCreateXML, networkDestroy, networkIsActive and networkLookupByName.
> 
> In the function phypCreateNetwork I just use the def->domain information to create
> the new network interface because of the behaviour of the HMC and the hypervisor:
> 
>     * HMC can't simply create a network interface without assigning it to a specific 
>       LPAR.
>     * I also can't assign an IP addr or any other information from the HMC or VIOS
>       side, but I can control in which vlan or vswitch it will be attached - but
>       thought just in the simplest case scenarion now, I'll make some improvements

s/scenarion/scenario/

> +++ b/src/phyp/phyp_driver.c
> @@ -1,7 +1,7 @@
>  
>  /*
>   * Copyright (C) 2010 Red Hat, Inc.
> - * Copyright IBM Corp. 2009
> + * Copyright IBM Corp. 2010

You should never remove copyright years; this would be okay as 2009-2010.

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

Another grep | sed you can simplify:

sed -n '/%s/ s/^.*,//p'

> +    virBufferVSprintf(&buf,
> +                      " -r virtualio --rsubtype eth --level lpar "
> +                      " -F mac_addr,slot_num|grep %lld|"
> +                      " sed -e 's/^.*,//g'", mac);

MACs are usually represented as hex, not decimal.  And if it really is
decimal, wouldn't you want unsigned?

Simplify:

sed -n '/%lld/ /^.*,//p'

> +
> +    if (!def->domain) {
> +        VIR_ERROR0(_("Domain can't be NULL, you must especify in which"

s/especify/specify/

> +
> +    ret = phypExec(session, cmd, &exit_status, conn);
> +
> +    if (exit_status < 0 || ret != NULL)
> +        goto err;
> +
> +    /* Need to sleep a little while to wait for the HMC to
> +     * complete the execution of the command.
> +     * */
> +    sleep(1);

This seems racy, and 1 second is a long pause.  Is there something more
reliable you can use to tell whether HMC is done?  Can you set up a
retry loop and sleep for shorter periods of time with retries until it
works to avoid a long pause?

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

sed '/lpar_id=%d/!d; /slot_num=%d/!d; s/^.*drc_name=//'

> +    /* Getting the new interface mac addr */
> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      "-r virtualio --rsubtype eth --level lpar "
> +                      "|grep lpar_id=%d|grep slot_num=%d|"
> +                      " sed -e 's/^.*mac_addr=//g'", lpar_id, slot);

Similar.

> +    virBufferAddLit(&buf, "lshwres ");
> +    if (system_type == HMC)
> +        virBufferVSprintf(&buf, "-m %s ", managed_system);
> +
> +    virBufferVSprintf(&buf,
> +                      "-r virtualio --rsubtype eth --level lpar "
> +                      "-F mac_addr,state |grep %lld|"
> +                      "sed -e 's/^.*,//g'", mac);

Another instance where decimal MAC seems odd.

sed -n '/%lld/ s/^.*,//p'

> diff --git a/src/phyp/phyp_driver.h b/src/phyp/phyp_driver.h
> index 603d048..34ad84b 100644
> --- a/src/phyp/phyp_driver.h
> +++ b/src/phyp/phyp_driver.h
> @@ -1,6 +1,6 @@
>  /*
>   * Copyright (C) 2010 Red Hat, Inc.
> - * Copyright IBM Corp. 2009
> + * Copyright IBM Corp. 2010

This hunk seems completely random.  Should it be rebased into another
patch that actually touches phyp_driver.h?  And should it be 2009-2010?

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