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@xxxxxxxxxx +1-801-349-2682
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