* [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
@ 2013-11-04 18:06 alexandrasava18
2013-11-04 18:33 ` Wei Liu
2013-11-04 21:43 ` Alexandra Sava
0 siblings, 2 replies; 8+ messages in thread
From: alexandrasava18 @ 2013-11-04 18:06 UTC (permalink / raw)
To: xen-devel; +Cc: anthony.perard, Alexandra Sava
From: Alexandra Sava <alexandrasava18@gmail.com>
---
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));
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));
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);
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",
+ 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",
+ 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));
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));
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);
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);
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));
}
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);
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));
goto out;
}
@@ -941,7 +927,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
default:
/* everything else is an error */
LOG(ERROR, "unable to get args/env to execute hotplug script for "
- "device %s", libxl__device_backend_path(gc, aodev->dev));
+ "device %s", libxl__device_backend_path(gc, aodev->dev));
rc = hotplug;
goto out;
}
@@ -1087,7 +1073,7 @@ int libxl__wait_for_device_model(libxl__gc *gc,
void *check_callback_userdata)
{
char *path;
- path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+ path = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
return libxl__wait_for_offspring(gc, domid,
LIBXL_DEVICE_MODEL_START_TIMEOUT,
"Device Model", path, state, spawning,
@@ -1096,22 +1082,20 @@ int libxl__wait_for_device_model(libxl__gc *gc,
int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl_ctx *ctx = CTX;
int watchdog = 100;
unsigned int len;
char *p;
- char *path = libxl__sprintf(gc, "%s/state", be_path);
+ char *path = GCSPRINTF("%s/state", be_path);
int rc = -1;
while (watchdog > 0) {
p = xs_read(ctx->xsh, XBT_NULL, path, &len);
if (p == NULL) {
if (errno == ENOENT) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s does not exist",
- be_path);
+ LOG(ERROR, "Backend %s does not exist", be_path);
} else {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Failed to access backend %s",
- be_path);
+ LOGE(ERROR, "Failed to access backend %s", be_path);
}
goto out;
} else {
@@ -1124,7 +1108,7 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
}
}
}
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s not ready", be_path);
+ LOG(ERROR, "Backend %s not ready", be_path);
out:
return rc;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-04 18:06 [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c alexandrasava18
@ 2013-11-04 18:33 ` Wei Liu
2013-11-04 21:43 ` Alexandra Sava
1 sibling, 0 replies; 8+ messages in thread
From: Wei Liu @ 2013-11-04 18:33 UTC (permalink / raw)
To: alexandrasava18; +Cc: anthony.perard, wei.liu2, xen-devel
On Mon, Nov 04, 2013 at 08:06:44PM +0200, alexandrasava18@gmail.com wrote:
> From: Alexandra Sava <alexandrasava18@gmail.com>
>
You need to sign off this patch. See
http://wiki.xen.org/wiki/Submitting_Xen_Patches
Wei.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-04 18:06 [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c alexandrasava18
2013-11-04 18:33 ` Wei Liu
@ 2013-11-04 21:43 ` Alexandra Sava
2013-11-05 11:48 ` George Dunlap
1 sibling, 1 reply; 8+ messages in thread
From: Alexandra Sava @ 2013-11-04 21:43 UTC (permalink / raw)
To: xen-devel; +Cc: anthony.perard, Alexandra Sava
Signed-off-by: Alexandra Sava <alexandrasava18@gmail.com>
---
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));
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));
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);
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",
+ 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",
+ 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));
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));
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);
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);
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));
}
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);
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));
goto out;
}
@@ -941,7 +927,7 @@ static void device_hotplug(libxl__egc *egc, libxl__ao_device *aodev)
default:
/* everything else is an error */
LOG(ERROR, "unable to get args/env to execute hotplug script for "
- "device %s", libxl__device_backend_path(gc, aodev->dev));
+ "device %s", libxl__device_backend_path(gc, aodev->dev));
rc = hotplug;
goto out;
}
@@ -1087,7 +1073,7 @@ int libxl__wait_for_device_model(libxl__gc *gc,
void *check_callback_userdata)
{
char *path;
- path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+ path = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
return libxl__wait_for_offspring(gc, domid,
LIBXL_DEVICE_MODEL_START_TIMEOUT,
"Device Model", path, state, spawning,
@@ -1096,22 +1082,20 @@ int libxl__wait_for_device_model(libxl__gc *gc,
int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
{
- libxl_ctx *ctx = libxl__gc_owner(gc);
+ libxl_ctx *ctx = CTX;
int watchdog = 100;
unsigned int len;
char *p;
- char *path = libxl__sprintf(gc, "%s/state", be_path);
+ char *path = GCSPRINTF("%s/state", be_path);
int rc = -1;
while (watchdog > 0) {
p = xs_read(ctx->xsh, XBT_NULL, path, &len);
if (p == NULL) {
if (errno == ENOENT) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s does not exist",
- be_path);
+ LOG(ERROR, "Backend %s does not exist", be_path);
} else {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Failed to access backend %s",
- be_path);
+ LOGE(ERROR, "Failed to access backend %s", be_path);
}
goto out;
} else {
@@ -1124,7 +1108,7 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
}
}
}
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s not ready", be_path);
+ LOG(ERROR, "Backend %s not ready", be_path);
out:
return rc;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-04 21:43 ` Alexandra Sava
@ 2013-11-05 11:48 ` George Dunlap
2013-11-05 12:02 ` Ian Campbell
2013-11-05 13:56 ` Ian Campbell
0 siblings, 2 replies; 8+ messages in thread
From: George Dunlap @ 2013-11-05 11:48 UTC (permalink / raw)
To: Alexandra Sava; +Cc: Anthony PERARD, Ian Jackson, xen-devel
On Mon, Nov 4, 2013 at 9:43 PM, Alexandra Sava
<alexandrasava18@gmail.com> wrote:
> Signed-off-by: Alexandra Sava <alexandrasava18@gmail.com>
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-05 11:48 ` George Dunlap
@ 2013-11-05 12:02 ` Ian Campbell
2013-11-05 13:56 ` Ian Campbell
1 sibling, 0 replies; 8+ messages in thread
From: Ian Campbell @ 2013-11-05 12:02 UTC (permalink / raw)
To: George Dunlap; +Cc: Anthony PERARD, Ian Jackson, Alexandra Sava, xen-devel
On Tue, 2013-11-05 at 11:48 +0000, George Dunlap wrote:
> On Mon, Nov 4, 2013 at 9:43 PM, Alexandra Sava
> <alexandrasava18@gmail.com> wrote:
> > Signed-off-by: Alexandra Sava <alexandrasava18@gmail.com>
>
> 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.
FWIW I read this as "Cleanup: use LOG*" which I think is accurate.
> > @@ -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;
The intended use of CTX is inline in the code, not to create a lower
case convenience. This is probably a separate cleanup though.
> > char *frontend_path, *backend_path;
> > struct xs_permissions frontend_perms[2];
> > struct xs_permissions ro_frontend_perms[2];
> @@ -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.
Or maybe re-break it at the punctuation so it is greppable (my main
bugbear with the 80 column rule is that it causes people to break these
strings in a way which makes them unfindable...)
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-05 11:48 ` George Dunlap
2013-11-05 12:02 ` Ian Campbell
@ 2013-11-05 13:56 ` Ian Campbell
2013-11-05 14:34 ` Alexandra Sava
1 sibling, 1 reply; 8+ messages in thread
From: Ian Campbell @ 2013-11-05 13:56 UTC (permalink / raw)
To: George Dunlap; +Cc: Anthony PERARD, Ian Jackson, Alexandra Sava, xen-devel
On Tue, 2013-11-05 at 11:48 +0000, George Dunlap wrote:
> > 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,
There's a bit of a mixture and the indented version is more prevalent.
But there's only 7 multiline uses of LOG* in that file (the split is 5:2
in favour of indentation).
I'd object to just reindenting but if the line is changing anyway I have
no strong feelings on the style used.
The non-indented variant does have the benefit that formatting helpers
(e.g. emacs' c-mode) do it that way for you...
Ian.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
2013-11-05 13:56 ` Ian Campbell
@ 2013-11-05 14:34 ` Alexandra Sava
2013-11-05 16:55 ` [PATCH v2] libxl: Use new LOG* and GCSPRINTF " Alexandra Sava
0 siblings, 1 reply; 8+ messages in thread
From: Alexandra Sava @ 2013-11-05 14:34 UTC (permalink / raw)
To: Ian Campbell; +Cc: George Dunlap, Anthony PERARD, Ian Jackson, xen-devel
On 5 November 2013 15:56, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-11-05 at 11:48 +0000, George Dunlap wrote:
>> > 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,
>
> There's a bit of a mixture and the indented version is more prevalent.
> But there's only 7 multiline uses of LOG* in that file (the split is 5:2
> in favour of indentation).
>
> I'd object to just reindenting but if the line is changing anyway I have
> no strong feelings on the style used.
>
> The non-indented variant does have the benefit that formatting helpers
> (e.g. emacs' c-mode) do it that way for you...
>
> Ian.
>
>
Hi guys,
Thanks for the review. I will update the patch based on your suggestions:
* fix indentation stuff
* break lines up to 80 characters when necessary
* change commit message with:
Use new LOG* and GCSPRINTF macros in libxl_device.c
Replace libxl__sprintf, LIBXL__LOG and LIBXL__LOG_ERRNO with new
"Convenience macros": GCSPRINTF, LOG, LOGE
Thanks,
Alexandra
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] libxl: Use new LOG* and GCSPRINTF macros in libxl_device.c
2013-11-05 14:34 ` Alexandra Sava
@ 2013-11-05 16:55 ` Alexandra Sava
0 siblings, 0 replies; 8+ messages in thread
From: Alexandra Sava @ 2013-11-05 16:55 UTC (permalink / raw)
To: xen-devel; +Cc: anthony.perard, George.Dunlap, ian.jackson, Alexandra Sava
Replace libxl__sprintf, LIBXL__LOG and LIBXL__LOG_ERRNO with new
"Convenience macros": GCSPRINTF, LOG, LOGE
Signed-off-by: Alexandra Sava <alexandrasava18@gmail.com>
---
tools/libxl/libxl_device.c | 95 +++++++++++++++++++-------------------------
1 file changed, 40 insertions(+), 55 deletions(-)
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 16a92a4..56c7099 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,
@@ -125,7 +125,8 @@ 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));
if (fents)
libxl__xs_writev_perms(gc, t, frontend_path, fents,
frontend_perms, ARRAY_SIZE(frontend_perms));
@@ -138,7 +139,8 @@ 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));
libxl__xs_writev(gc, t, backend_path, bents);
}
@@ -149,7 +151,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 +170,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:
@@ -192,24 +193,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", 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",
+ 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", a->disk->vdev);
return 0;
}
if (!(a->disk->format == LIBXL_DISK_FORMAT_RAW ||
@@ -223,16 +221,14 @@ 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",
+ 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));
@@ -245,22 +241,18 @@ 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,
+ LOG(DEBUG, "Disk vdev=%s spec.backend=%s", disk->vdev,
libxl_disk_backend_to_string(disk->backend));
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 +261,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);
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);
return ERROR_INVAL;
}
}
@@ -291,13 +281,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",
+ LOG(DEBUG, "Disk vdev=%s, using backend %s",
disk->vdev,
libxl_disk_backend_to_string(ok));
}
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 +584,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 +597,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 +610,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 +641,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 +701,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 +749,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;
@@ -1087,7 +1074,7 @@ int libxl__wait_for_device_model(libxl__gc *gc,
void *check_callback_userdata)
{
char *path;
- path = libxl__sprintf(gc, "/local/domain/0/device-model/%d/state", domid);
+ path = GCSPRINTF("/local/domain/0/device-model/%d/state", domid);
return libxl__wait_for_offspring(gc, domid,
LIBXL_DEVICE_MODEL_START_TIMEOUT,
"Device Model", path, state, spawning,
@@ -1100,18 +1087,16 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
int watchdog = 100;
unsigned int len;
char *p;
- char *path = libxl__sprintf(gc, "%s/state", be_path);
+ char *path = GCSPRINTF("%s/state", be_path);
int rc = -1;
while (watchdog > 0) {
p = xs_read(ctx->xsh, XBT_NULL, path, &len);
if (p == NULL) {
if (errno == ENOENT) {
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s does not exist",
- be_path);
+ LOG(ERROR, "Backend %s does not exist", be_path);
} else {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Failed to access backend %s",
- be_path);
+ LOGE(ERROR, "Failed to access backend %s", be_path);
}
goto out;
} else {
@@ -1124,7 +1109,7 @@ int libxl__wait_for_backend(libxl__gc *gc, char *be_path, char *state)
}
}
}
- LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Backend %s not ready", be_path);
+ LOG(ERROR, "Backend %s not ready", be_path);
out:
return rc;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-05 16:55 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-04 18:06 [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c alexandrasava18
2013-11-04 18:33 ` Wei Liu
2013-11-04 21:43 ` Alexandra Sava
2013-11-05 11:48 ` George Dunlap
2013-11-05 12:02 ` Ian Campbell
2013-11-05 13:56 ` Ian Campbell
2013-11-05 14:34 ` Alexandra Sava
2013-11-05 16:55 ` [PATCH v2] libxl: Use new LOG* and GCSPRINTF " Alexandra Sava
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.