|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
On Mon, Nov 4, 2013 at 9:43 PM, Alexandra Sava
<alexandrasava18@xxxxxxxxx> wrote:
> Signed-off-by: Alexandra Sava <alexandrasava18@xxxxxxxxx>
Thanks for submitting this, Alexandra -- it's the kind of thing that
is important but we rarely make the time to do.
I don't think the description is quite accurate: you're not cleaning
up existing use of those macros, but you're updating code to use the
new macros.
For future reference, I think this patch would probably be a bit
easier to review if it was broken up into 3, one for each macro, so
that we don't have to keep switching gears when evaluating if a change
is probably correct.
> ---
> tools/libxl/libxl_device.c | 122
> +++++++++++++++++++-------------------------
> 1 file changed, 53 insertions(+), 69 deletions(-)
>
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 16a92a4..b36fb13 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -24,20 +24,20 @@ char *libxl__device_frontend_path(libxl__gc *gc,
> libxl__device *device)
>
> /* Console 0 is a special case */
> if (device->kind == LIBXL__DEVICE_KIND_CONSOLE && device->devid == 0)
> - return libxl__sprintf(gc, "%s/console", dom_path);
> + return GCSPRINTF("%s/console", dom_path);
>
> - return libxl__sprintf(gc, "%s/device/%s/%d", dom_path,
> - libxl__device_kind_to_string(device->kind),
> - device->devid);
> + return GCSPRINTF("%s/device/%s/%d", dom_path,
> + libxl__device_kind_to_string(device->kind),
> + device->devid);
> }
>
> char *libxl__device_backend_path(libxl__gc *gc, libxl__device *device)
> {
> char *dom_path = libxl__xs_get_dompath(gc, device->backend_domid);
>
> - return libxl__sprintf(gc, "%s/backend/%s/%u/%d", dom_path,
> - libxl__device_kind_to_string(device->backend_kind),
> - device->domid, device->devid);
> + return GCSPRINTF("%s/backend/%s/%u/%d", dom_path,
> + libxl__device_kind_to_string(device->backend_kind),
> + device->domid, device->devid);
> }
>
> int libxl__parse_backend_path(libxl__gc *gc,
> @@ -86,7 +86,7 @@ out:
> int libxl__device_generic_add(libxl__gc *gc, xs_transaction_t t,
> libxl__device *device, char **bents, char **fents, char **ro_fents)
> {
> - libxl_ctx *ctx = libxl__gc_owner(gc);
> + libxl_ctx *ctx = CTX;
> char *frontend_path, *backend_path;
> struct xs_permissions frontend_perms[2];
> struct xs_permissions ro_frontend_perms[2];
> @@ -125,7 +125,7 @@ retry_transaction:
> else
> xs_set_permissions(ctx->xsh, t, frontend_path,
> frontend_perms, ARRAY_SIZE(frontend_perms));
> - xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/backend",
> frontend_path), backend_path, strlen(backend_path));
> + xs_write(ctx->xsh, t, GCSPRINTF("%s/backend", frontend_path),
> backend_path, strlen(backend_path));
While we're here, it might be nice to break this line down so that
it's only 80 columns.
> if (fents)
> libxl__xs_writev_perms(gc, t, frontend_path, fents,
> frontend_perms,
> ARRAY_SIZE(frontend_perms));
> @@ -138,7 +138,7 @@ retry_transaction:
> xs_rm(ctx->xsh, t, backend_path);
> xs_mkdir(ctx->xsh, t, backend_path);
> xs_set_permissions(ctx->xsh, t, backend_path, backend_perms,
> ARRAY_SIZE(backend_perms));
> - xs_write(ctx->xsh, t, libxl__sprintf(gc, "%s/frontend",
> backend_path), frontend_path, strlen(frontend_path));
> + xs_write(ctx->xsh, t, GCSPRINTF("%s/frontend", backend_path),
> frontend_path, strlen(frontend_path));
Same here.
> libxl__xs_writev(gc, t, backend_path, bents);
> }
>
> @@ -149,7 +149,7 @@ retry_transaction:
> if (errno == EAGAIN)
> goto retry_transaction;
> else {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xs transaction failed");
> + LOGE(ERROR, "xs transaction failed");
> return ERROR_FAIL;
> }
> }
> @@ -168,7 +168,6 @@ static int disk_try_backend(disk_try_backend_args *a,
> libxl__gc *gc = a->gc;
> /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or
> * backend on success */
> - libxl_ctx *ctx = libxl__gc_owner(gc);
>
> switch (backend) {
> case LIBXL_DISK_BACKEND_PHY:
> @@ -179,7 +178,7 @@ static int disk_try_backend(disk_try_backend_args *a,
>
> if (a->disk->backend_domid != LIBXL_TOOLSTACK_DOMID) {
> LOG(DEBUG, "Disk vdev=%s, is using a storage driver domain, "
> - "skipping physical device check", a->disk->vdev);
> + "skipping physical device check", a->disk->vdev);
Why did you change the indentation here?
> return backend;
> }
>
> @@ -192,24 +191,21 @@ static int disk_try_backend(disk_try_backend_args *a,
> if (libxl__try_phy_backend(a->stab.st_mode))
> return backend;
>
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
> - " unsuitable as phys path not a block device",
> - a->disk->vdev);
> + LOG(DEBUG, "Disk vdev=%s, backend phy unsuitable as phys path not a
> block device",
This line is now more than 80 characters long -- please leave it
broken down so as to be less than 80 characters long.
> + a->disk->vdev);
> return 0;
>
> case LIBXL_DISK_BACKEND_TAP:
> if (a->disk->script) goto bad_script;
>
> if (a->disk->is_cdrom) {
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
> - " unsuitable for cdroms",
> - a->disk->vdev);
> + LOG(DEBUG, "Disk vdev=%s, backend tap unsuitable for cdroms",
> + a->disk->vdev);
> return 0;
> }
> if (!libxl__blktap_enabled(a->gc)) {
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend tap"
> - " unsuitable because blktap not available",
> - a->disk->vdev);
> + LOG(DEBUG, "Disk vdev=%s, backend tap unsuitable because blktap
> not available",
Same here.
> + a->disk->vdev);
> return 0;
> }
> if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
> @@ -223,19 +219,17 @@ static int disk_try_backend(disk_try_backend_args *a,
> return backend;
>
> default:
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend "
> - " %d unknown", a->disk->vdev, backend);
> + LOG(DEBUG, "Disk vdev=%s, backend %d unknown", a->disk->vdev,
> backend);
> return 0;
>
> }
> abort(); /* notreached */
>
> bad_format:
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend %s"
> - " unsuitable due to format %s",
> - a->disk->vdev,
> - libxl_disk_backend_to_string(backend),
> - libxl_disk_format_to_string(a->disk->format));
> + LOG(DEBUG, "Disk vdev=%s, backend %s unsuitable due to format %s",
> + a->disk->vdev,
> + libxl_disk_backend_to_string(backend),
> + libxl_disk_format_to_string(a->disk->format));
Given that in existing places, the LOG macro has formatted variables
aligned to the beginning of the quote rather than to the open paren,
it's probably better to follow suit when changing the macro over.
(Ian Jackson may have opinions on this.)
> return 0;
>
> bad_script:
> @@ -245,22 +239,19 @@ static int disk_try_backend(disk_try_backend_args *a,
> }
>
> int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
> - libxl_ctx *ctx = libxl__gc_owner(gc);
> libxl_disk_backend ok;
> disk_try_backend_args a;
>
> a.gc = gc;
> a.disk = disk;
>
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s spec.backend=%s",
> - disk->vdev,
> - libxl_disk_backend_to_string(disk->backend));
> + LOG(DEBUG, "Disk vdev=%s spec.backend=%s",
> + disk->vdev,
> + libxl_disk_backend_to_string(disk->backend));
Alignment (i.e., w/ '"' rather than '(')
>
> if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
> if (!disk->is_cdrom) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
> - " but not cdrom",
> - disk->vdev);
> + LOG(ERROR, "Disk vdev=%s is empty but not cdrom", disk->vdev);
> return ERROR_INVAL;
> }
> memset(&a.stab, 0, sizeof(a.stab));
> @@ -269,16 +260,14 @@ int libxl__device_disk_set_backend(libxl__gc *gc,
> libxl_device_disk *disk) {
> disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
> !disk->script) {
> if (stat(disk->pdev_path, &a.stab)) {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
> - "failed to stat: %s",
> - disk->vdev, disk->pdev_path);
> + LOGE(ERROR, "Disk vdev=%s failed to stat: %s",
> + disk->vdev, disk->pdev_path);
Alignment
> return ERROR_INVAL;
> }
> if (!S_ISBLK(a.stab.st_mode) &
> !S_ISREG(a.stab.st_mode)) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
> - "phys path is not a block dev or file: %s",
> - disk->vdev, disk->pdev_path);
> + LOG(ERROR, "Disk vdev=%s phys path is not a block dev or file:
> %s",
> + disk->vdev, disk->pdev_path);
Alignment
> return ERROR_INVAL;
> }
> }
> @@ -291,13 +280,12 @@ int libxl__device_disk_set_backend(libxl__gc *gc,
> libxl_device_disk *disk) {
> disk_try_backend(&a, LIBXL_DISK_BACKEND_QDISK) ?:
> disk_try_backend(&a, LIBXL_DISK_BACKEND_TAP);
> if (ok)
> - LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, using backend
> %s",
> - disk->vdev,
> - libxl_disk_backend_to_string(ok));
> + LOG(DEBUG, "Disk vdev=%s, using backend %s",
> + disk->vdev,
> + libxl_disk_backend_to_string(ok));
Alignment
> }
> if (!ok) {
> - LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "no suitable backend for disk %s",
> - disk->vdev);
> + LOG(ERROR, "no suitable backend for disk %s", disk->vdev);
> return ERROR_INVAL;
> }
> disk->backend = ok;
> @@ -595,7 +583,6 @@ static void devices_remove_callback(libxl__egc *egc,
> void libxl__devices_destroy(libxl__egc *egc, libxl__devices_remove_state
> *drs)
> {
> STATE_AO_GC(drs->ao);
> - libxl_ctx *ctx = libxl__gc_owner(gc);
> uint32_t domid = drs->domid;
> char *path;
> unsigned int num_kinds, num_dev_xsentries;
> @@ -609,12 +596,11 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> libxl__multidev_begin(ao, multidev);
> multidev->callback = devices_remove_callback;
>
> - path = libxl__sprintf(gc, "/local/domain/%d/device", domid);
> + path = GCSPRINTF("/local/domain/%d/device", domid);
> kinds = libxl__xs_directory(gc, XBT_NULL, path, &num_kinds);
> if (!kinds) {
> if (errno != ENOENT) {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "unable to get xenstore"
> - " device listing %s", path);
> + LOGE(ERROR, "unable to get xenstore device listing %s", path);
> goto out;
> }
> num_kinds = 0;
> @@ -623,13 +609,13 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> if (libxl__device_kind_from_string(kinds[i], &kind))
> continue;
>
> - path = libxl__sprintf(gc, "/local/domain/%d/device/%s", domid,
> kinds[i]);
> + path = GCSPRINTF("/local/domain/%d/device/%s", domid, kinds[i]);
> devs = libxl__xs_directory(gc, XBT_NULL, path, &num_dev_xsentries);
> if (!devs)
> continue;
> for (j = 0; j < num_dev_xsentries; j++) {
> - path = libxl__sprintf(gc,
> "/local/domain/%d/device/%s/%s/backend",
> - domid, kinds[i], devs[j]);
> + path = GCSPRINTF("/local/domain/%d/device/%s/%s/backend",
> + domid, kinds[i], devs[j]);
> path = libxl__xs_read(gc, XBT_NULL, path);
> GCNEW(dev);
> if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
> @@ -654,7 +640,7 @@ void libxl__devices_destroy(libxl__egc *egc,
> libxl__devices_remove_state *drs)
> }
>
> /* console 0 frontend directory is not under
> /local/domain/<domid>/device */
> - path = libxl__sprintf(gc, "/local/domain/%d/console/backend", domid);
> + path = GCSPRINTF("/local/domain/%d/console/backend", domid);
> path = libxl__xs_read(gc, XBT_NULL, path);
> GCNEW(dev);
> if (path && strcmp(path, "") &&
> @@ -714,7 +700,7 @@ void libxl__wait_device_connection(libxl__egc *egc,
> libxl__ao_device *aodev)
> {
> STATE_AO_GC(aodev->ao);
> char *be_path = libxl__device_backend_path(gc, aodev->dev);
> - char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> + char *state_path = GCSPRINTF("%s/state", be_path);
> libxl_dominfo info;
> uint32_t domid = aodev->dev->domid;
> int rc = 0;
> @@ -762,7 +748,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
> STATE_AO_GC(aodev->ao);
> xs_transaction_t t = 0;
> char *be_path = libxl__device_backend_path(gc, aodev->dev);
> - char *state_path = libxl__sprintf(gc, "%s/state", be_path);
> + char *state_path = GCSPRINTF("%s/state", be_path);
> char *online_path = GCSPRINTF("%s/online", be_path);
> const char *state;
> libxl_dominfo info;
> @@ -786,7 +772,7 @@ void libxl__initiate_device_remove(libxl__egc *egc,
> LIBXL_QEMU_BODGE_TIMEOUT * 1000);
> if (rc) {
> LOG(ERROR, "unable to register timeout for Qemu device %s",
> - be_path);
> + be_path);
Alignment
> goto out;
> }
> return;
> @@ -890,8 +876,8 @@ static void device_backend_callback(libxl__egc *egc,
> libxl__ev_devstate *ds,
>
> if (rc) {
> LOG(ERROR, "unable to %s device with path %s",
> - libxl__device_action_to_string(aodev->action),
> - libxl__device_backend_path(gc, aodev->dev));
> + libxl__device_action_to_string(aodev->action),
> + libxl__device_backend_path(gc, aodev->dev));
Alignment
Everything else looks good to me. Thanks!
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |