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

Re: [libvirt] PATCH: 5/7: Remove paths from virDomainObjPtr



"Daniel P. Berrange" <berrange redhat com> wrote:
...

ACK, modulo two questions:

> diff -r a204a9425afd src/domain_conf.c
>  int virDomainDeleteConfig(virConnectPtr conn,
> -                           virDomainObjPtr dom)
> +                          const char *configDir,
> +                          const char *autostartDir,
> +                          virDomainObjPtr dom)
>  {
> -    if (!dom->configFile || !dom->autostartLink) {
> -        virDomainReportError(conn, VIR_ERR_INTERNAL_ERROR,
> -                              _("no config file for %s"), dom->def->name);
> -        return -1;
> +    char *configFile = NULL, *autostartLink = NULL;
> +    int ret = -1;
> +
> +    if (asprintf(&configFile, "%s/%s",

Shouldn't that be "%s/%s.xml", as used in at least two
other places where configFile is defined?
This deserves a tiny helper function, so that the
dom->configFile mapping happens in just one place.

> +                 configDir, dom->def->name) < 0) {
> +        configFile = NULL;
...

>  #endif /* ! PROXY */
...
> diff -r a204a9425afd src/qemu_driver.c
...
>  static int qemudDomainSetAutostart(virDomainPtr dom,
> -                            int autostart) {
> +                                   int autostart) {
>      struct qemud_driver *driver = (struct qemud_driver *)dom->conn->privateData;
>      virDomainObjPtr vm = virDomainFindByUUID(driver->domains, dom->uuid);
> +    char *configFile = NULL, *autostartLink = NULL;
> +    int ret = -1;
>
>      if (!vm) {
>          qemudReportError(dom->conn, dom, NULL, VIR_ERR_INVALID_DOMAIN,
>                           "%s", _("no domain with matching uuid"));
> +        return -1;
> +    }
> +
> +    if (!vm->persistent) {
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR,
> +                         "%s", _("cannot set autostart for transient domain"));
>          return -1;
>      }
>
> @@ -3039,6 +3053,20 @@
>      if (vm->autostart == autostart)
>          return 0;
>
> +    if (asprintf(&configFile, "%s/%s.xml",
> +                 driver->configDir, vm->def->name) < 0) {
> +        configFile = NULL;
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY, NULL);
> +        goto cleanup;
> +    }
> +
> +    if (asprintf(&autostartLink, "%s/%s.xml",
> +                 driver->autostartDir, vm->def->name) < 0) {

Your 6/7 patch has this:

    if (asprintf(&autostartLink, "%s/%s",

Is the different format deliberate?
Maybe a helper function for the dom->autostartLink name, too?

> +        autostartLink = NULL;
> +        qemudReportError(dom->conn, dom, NULL, VIR_ERR_NO_MEMORY, NULL);
> +        goto cleanup;
> +    }
> +
...
> +cleanup:
> +    VIR_FREE(configFile);
> +    VIR_FREE(autostartLink);
> +
> +    return ret;
>  }
>
>  /* This uses the 'info blockstats' monitor command which was


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