alexandrasava18@gmail.com
2013-Nov-04 18:06 UTC
[PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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
Wei Liu
2013-Nov-04 18:33 UTC
Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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.
Alexandra Sava
2013-Nov-04 21:43 UTC
[PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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
George Dunlap
2013-Nov-05 11:48 UTC
Re: [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@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
Ian Campbell
2013-Nov-05 12:02 UTC
Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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.
Ian Campbell
2013-Nov-05 13:56 UTC
Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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.
Alexandra Sava
2013-Nov-05 14:34 UTC
Re: [PATCH] libxl: Cleanup, use LOG*, GCSPRINTF and CTX macros in libxl_device.c
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
Alexandra Sava
2013-Nov-05 16:55 UTC
[PATCH v2] libxl: 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
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