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

Re: [libvirt] [PATCH 05/15] Move veth device management into virLXCControllerPtr object



On Tue, Jul 03, 2012 at 16:58:44 +0100, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
> 
> Move the veth device name state into the virLXCControllerPtr
> object and stop passing it around. Also use size_t instead
> of unsigned int for the array length parameters.
> 
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  src/lxc/lxc_container.c  |   10 +++---
>  src/lxc/lxc_container.h  |    2 +-
>  src/lxc/lxc_controller.c |   79 ++++++++++++++++++++++++++++++----------------
>  3 files changed, 57 insertions(+), 34 deletions(-)
> 
> diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c
> index 910e82b..4f8703c 100644
> --- a/src/lxc/lxc_container.c
> +++ b/src/lxc/lxc_container.c
> @@ -93,7 +93,7 @@ typedef struct __lxc_child_argv lxc_child_argv_t;
>  struct __lxc_child_argv {
>      virDomainDefPtr config;
>      virSecurityManagerPtr securityDriver;
> -    unsigned int nveths;
> +    size_t nveths;
>      char **veths;
>      int monitor;
>      char **ttyPaths;
> @@ -262,15 +262,15 @@ int lxcContainerWaitForContinue(int control)
>   * Returns 0 on success or nonzero in case of error
>   */
>  static int lxcContainerRenameAndEnableInterfaces(bool privNet,
> -                                                 unsigned int nveths,
> +                                                 size_t nveths,
>                                                   char **veths)
>  {
>      int rc = 0;
> -    unsigned int i;
> +    size_t i;
>      char *newname = NULL;
>  
>      for (i = 0 ; i < nveths ; i++) {
> -        if (virAsprintf(&newname, "eth%d", i) < 0) {
> +        if (virAsprintf(&newname, "eth%zu", i) < 0) {
>              virReportOOMError();
>              rc = -1;
>              goto error_out;
> @@ -1775,7 +1775,7 @@ const char *lxcContainerGetAlt32bitArch(const char *arch)
>   */
>  int lxcContainerStart(virDomainDefPtr def,
>                        virSecurityManagerPtr securityDriver,
> -                      unsigned int nveths,
> +                      size_t nveths,
>                        char **veths,
>                        int control,
>                        int handshakefd,
> diff --git a/src/lxc/lxc_container.h b/src/lxc/lxc_container.h
> index 77fb9b2..6f1cc8b 100644
> --- a/src/lxc/lxc_container.h
> +++ b/src/lxc/lxc_container.h
> @@ -51,7 +51,7 @@ int lxcContainerWaitForContinue(int control);
>  
>  int lxcContainerStart(virDomainDefPtr def,
>                        virSecurityManagerPtr securityDriver,
> -                      unsigned int nveths,
> +                      size_t nveths,
>                        char **veths,
>                        int control,
>                        int handshakefd,
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 7447f6c..ad11307 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -85,6 +85,9 @@ typedef virLXCController *virLXCControllerPtr;
>  struct _virLXCController {
>      char *name;
>      virDomainDefPtr def;
> +
> +    size_t nveths;
> +    char **veths;
>  };
>  
>  static void virLXCControllerFree(virLXCControllerPtr ctrl);
> @@ -129,15 +132,35 @@ no_memory:
>  
>  static void virLXCControllerFree(virLXCControllerPtr ctrl)
>  {
> +    size_t i;
> +
>      if (!ctrl)
>          return;
>  
> +    for (i = 0 ; i < ctrl->nveths ; i++)
> +        VIR_FREE(ctrl->veths[i]);
> +    VIR_FREE(ctrl->veths);
> +
>      virDomainDefFree(ctrl->def);
>      VIR_FREE(ctrl->name);
>  
>      VIR_FREE(ctrl);
>  }
>  
> +
> +static int virLXCControllerValidateNICs(virLXCControllerPtr ctrl)
> +{
> +    if (ctrl->def->nnets != ctrl->nveths) {
> +        lxcError(VIR_ERR_INTERNAL_ERROR,
> +                 _("expecting %d veths, but got %zu"),
> +                 ctrl->def->nnets, ctrl->nveths);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  static int lxcGetLoopFD(char **dev_name)
>  {
>      int fd = -1;
> @@ -1307,7 +1330,7 @@ cleanup2:
>  
>  
>  /**
> - * lxcControllerMoveInterfaces
> + * virLXCControllerMoveInterfaces
>   * @nveths: number of interfaces
>   * @veths: interface names
>   * @container: pid of container
> @@ -1316,13 +1339,13 @@ cleanup2:
>   *
>   * Returns 0 on success or -1 in case of error
>   */
> -static int lxcControllerMoveInterfaces(unsigned int nveths,
> -                                       char **veths,
> -                                       pid_t container)
> +static int virLXCControllerMoveInterfaces(virLXCControllerPtr ctrl,
> +                                          pid_t container)
>  {
> -    unsigned int i;
> -    for (i = 0 ; i < nveths ; i++)
> -        if (virNetDevSetNamespace(veths[i], container) < 0)
> +    size_t i;
> +
> +    for (i = 0 ; i < ctrl->nveths ; i++)
> +        if (virNetDevSetNamespace(ctrl->veths[i], container) < 0)
>              return -1;

Since you are touching this, I think it would be a good idea to add {} around
the content of this for loop.

>  
>      return 0;
> @@ -1330,24 +1353,26 @@ static int lxcControllerMoveInterfaces(unsigned int nveths,
>  
>  
>  /**
> - * lxcCleanupInterfaces:
> - * @nveths: number of interfaces
> - * @veths: interface names
> + * virLXCControllerDeleteInterfaces:
> + * @ctrl: the LXC controller
>   *
>   * Cleans up the container interfaces by deleting the veth device pairs.
>   *
>   * Returns 0 on success or -1 in case of error
>   */
> -static int lxcControllerCleanupInterfaces(unsigned int nveths,
> -                                          char **veths)
> +static int virLXCControllerDeleteInterfaces(virLXCControllerPtr ctrl)
>  {
> -    unsigned int i;
> -    for (i = 0 ; i < nveths ; i++)
> -        ignore_value(virNetDevVethDelete(veths[i]));
> +    size_t i;
> +    int ret = 0;
>  
> -    return 0;
> +    for (i = 0 ; i < ctrl->nveths ; i++)
> +        if (virNetDevVethDelete(ctrl->veths[i]) < 0)
> +            ret = -1;

And here as well...

> +
> +    return ret;
>  }
>  
> +
>  static int lxcSetPersonality(virDomainDefPtr def)
>  {
>      struct utsname utsname;
> @@ -1422,8 +1447,6 @@ cleanup:
>  static int
>  virLXCControllerRun(virLXCControllerPtr ctrl,
>                      virSecurityManagerPtr securityDriver,
> -                    unsigned int nveths,
> -                    char **veths,
>                      int monitor,
>                      int client,
>                      int *ttyFDs,
> @@ -1588,8 +1611,8 @@ virLXCControllerRun(virLXCControllerPtr ctrl,
>  
>      if ((container = lxcContainerStart(ctrl->def,
>                                         securityDriver,
> -                                       nveths,
> -                                       veths,
> +                                       ctrl->nveths,
> +                                       ctrl->veths,
>                                         control[1],
>                                         containerhandshake[1],
>                                         containerTtyPaths,
> @@ -1598,7 +1621,7 @@ virLXCControllerRun(virLXCControllerPtr ctrl,
>      VIR_FORCE_CLOSE(control[1]);
>      VIR_FORCE_CLOSE(containerhandshake[1]);
>  
> -    if (lxcControllerMoveInterfaces(nveths, veths, container) < 0)
> +    if (virLXCControllerMoveInterfaces(ctrl, container) < 0)
>          goto cleanup;
>  
>      if (lxcContainerSendContinue(control[0]) < 0) {
> @@ -1684,7 +1707,7 @@ int main(int argc, char *argv[])
>      int rc = 1;
>      int client;
>      char *name = NULL;
> -    int nveths = 0;
> +    size_t nveths = 0;
>      char **veths = NULL;
>      int monitor = -1;
>      int handshakefd = -1;
> @@ -1833,11 +1856,11 @@ int main(int argc, char *argv[])
>                NULLSTR(ctrl->def->seclabel.label),
>                NULLSTR(ctrl->def->seclabel.imagelabel));
>  
> -    if (ctrl->def->nnets != nveths) {
> -        fprintf(stderr, "%s: expecting %d veths, but got %d\n",
> -                argv[0], ctrl->def->nnets, nveths);
> +    ctrl->veths = veths;
> +    ctrl->nveths = nveths;

I was a bit afraid of double free after the two lines above but since we
didn't actually free veths array before this patch (which was a little bit
wrong), we are safe.

> +
> +    if (virLXCControllerValidateNICs(ctrl) < 0)
>          goto cleanup;
> -    }
>  
>      if ((sockpath = lxcMonitorPath(ctrl)) == NULL)
>          goto cleanup;
> @@ -1885,12 +1908,12 @@ int main(int argc, char *argv[])
>      }
>  
>      rc = virLXCControllerRun(ctrl, securityDriver,
> -                             nveths, veths, monitor, client,
> +                             monitor, client,
>                               ttyFDs, nttyFDs, handshakefd);
>  
>  cleanup:
>      virPidFileDelete(LXC_STATE_DIR, name);
> -    lxcControllerCleanupInterfaces(nveths, veths);
> +    virLXCControllerDeleteInterfaces(ctrl);
>      if (sockpath)
>          unlink(sockpath);
>      VIR_FREE(sockpath);

ACK

Jirka


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