[PATCH 0/4] Compliance with new coding style

[PATCH 0/4] Compliance with new coding style

[Kelley Nielsen]

This patchset brings the file libxl_utils.c mostly into compliance
with the new coding style involving the convenience macros LOG,
LOGE, and GCSPRINTF.

[PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf
[PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in
[PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in
[PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in

[PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf

[Kelley Nielsen]

Code cleanup - no functional changes

The convenience macro GCSPRINTF has been written to be used in place
of libxl__sprintf(). Replace all calls to libxl__sprintf() in
libxl_utils.c with invocations of the new macro.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_utils.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 682f874..4841606 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -173,7 +173,7 @@ int libxl_get_stubdom_id(libxl_ctx *ctx, int guest_domid)
     int ret;
 
     stubdom_id_s = libxl__xs_read(gc, XBT_NULL,
-                                 libxl__sprintf(gc, "%s/image/device-model-domid",
+                                 GCSPRINTF("%s/image/device-model-domid",
                                                libxl__xs_get_dompath(gc, guest_domid)));
     if (stubdom_id_s)
         ret = atoi(stubdom_id_s);
@@ -190,7 +190,7 @@ int libxl_is_stubdom(libxl_ctx *ctx, uint32_t domid, uint32_t *target_domid)
     uint32_t value;
     int ret = 0;
 
-    target = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc, "%s/target", libxl__xs_get_dompath(gc, domid)));
+    target = libxl__xs_read(gc, XBT_NULL, GCSPRINTF("%s/target", libxl__xs_get_dompath(gc, domid)));
     if (!target)
         goto out;
     value = strtol(target, &endptr, 10);
@@ -227,20 +227,20 @@ int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name)
     char *logfile, *logfile_new;
     int i, rc;
 
-    logfile = libxl__sprintf(gc, "/var/log/xen/%s.log", name);
+    logfile = GCSPRINTF("/var/log/xen/%s.log", name);
     if (stat(logfile, &stat_buf) == 0) {
         /* file exists, rotate */
-        logfile = libxl__sprintf(gc, "/var/log/xen/%s.log.10", name);
+        logfile = GCSPRINTF("/var/log/xen/%s.log.10", name);
         unlink(logfile);
         for (i = 9; i > 0; i--) {
-            logfile = libxl__sprintf(gc, "/var/log/xen/%s.log.%d", name, i);
-            logfile_new = libxl__sprintf(gc, "/var/log/xen/%s.log.%d", name, i + 1);
+            logfile = GCSPRINTF("/var/log/xen/%s.log.%d", name, i);
+            logfile_new = GCSPRINTF("/var/log/xen/%s.log.%d", name, i + 1);
             rc = logrename(gc, logfile, logfile_new);
             if (rc)
                 goto out;
         }
-        logfile = libxl__sprintf(gc, "/var/log/xen/%s.log", name);
-        logfile_new = libxl__sprintf(gc, "/var/log/xen/%s.log.1", name);
+        logfile = GCSPRINTF("/var/log/xen/%s.log", name);
+        logfile_new = GCSPRINTF("/var/log/xen/%s.log.1", name);
 
         rc = logrename(gc, logfile, logfile_new);
         if (rc)
-- 
1.8.1.2

Re: [PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf

[Ian Campbell]

On Sun, 2013-11-10 at 23:08 -0800, Kelley Nielsen wrote:
> Code cleanup - no functional changes
> 
> The convenience macro GCSPRINTF has been written to be used in place
> of libxl__sprintf(). Replace all calls to libxl__sprintf() in
> libxl_utils.c with invocations of the new macro.
> 
> Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
> Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  tools/libxl/libxl_utils.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 682f874..4841606 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -173,7 +173,7 @@ int libxl_get_stubdom_id(libxl_ctx *ctx, int guest_domid)
>      int ret;
>  
>      stubdom_id_s = libxl__xs_read(gc, XBT_NULL,
> -                                 libxl__sprintf(gc, "%s/image/device-model-domid",
> +                                 GCSPRINTF("%s/image/device-model-domid",
>                                                 libxl__xs_get_dompath(gc, guest_domid)));

If you reindent this trailing line then:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

BTW, it looks like you are going to be resending a few minor revs of
both libxl_qmp.c and libxl_utils.c cleanup. Feel free to put them into a
single overarching "cleanup libxl" series for the resend. I assume it'll
be easier for you and it'll certainly be easier for us
reviewers/committers.

Thanks,
Ian.

[PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in libxl_utils.c

[Kelley Nielsen]

Code cleanup - no functional changes

The convenience macro LOGE has been written to take the place of
LIBXL__LOG_ERRNO. LOGE depends on the existence of a local libgl__gc
*gc. Replace two invocations of LIBXL__LOG_ERRNO, which are in
functions that already have a libxl__gc *gc present, to invocations
of the new macro.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_utils.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 4841606..5238f22 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -206,14 +206,13 @@ out:
 
 static int logrename(libxl__gc *gc, const char *old, const char *new)
 {
-    libxl_ctx *ctx = libxl__gc_owner(gc);
     int r;
 
     r = rename(old, new);
     if (r) {
         if (errno == ENOENT) return 0; /* ok */
 
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to rotate logfile - could not"
+        LOGE(ERROR, "failed to rotate logfile - could not"
                      " rename %s to %s", old, new);
         return ERROR_FAIL;
     }
@@ -247,7 +246,7 @@ int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name)
             goto out;
     } else {
         if (errno != ENOENT)
-            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING, "problem checking existence of"
+            LOGE(WARN, "problem checking existence of"
                          " logfile %s, which might have needed to be rotated",
                          name);
     }
-- 
1.8.1.2

Re: [PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in libxl_utils.c

[Ian Campbell]

On Sun, 2013-11-10 at 23:08 -0800, Kelley Nielsen wrote:
> Code cleanup - no functional changes
> 
> The convenience macro LOGE has been written to take the place of
> LIBXL__LOG_ERRNO. LOGE depends on the existence of a local libgl__gc
> *gc. Replace two invocations of LIBXL__LOG_ERRNO, which are in
> functions that already have a libxl__gc *gc present, to invocations
> of the new macro.
> 
> Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
> Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  tools/libxl/libxl_utils.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 4841606..5238f22 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -206,14 +206,13 @@ out:
>  
>  static int logrename(libxl__gc *gc, const char *old, const char *new)
>  {
> -    libxl_ctx *ctx = libxl__gc_owner(gc);
>      int r;
>  
>      r = rename(old, new);
>      if (r) {
>          if (errno == ENOENT) return 0; /* ok */
>  
> -        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to rotate logfile - could not"
> +        LOGE(ERROR, "failed to rotate logfile - could not"
>                       " rename %s to %s", old, new);
>          return ERROR_FAIL;
>      }
> @@ -247,7 +246,7 @@ int libxl_create_logfile(libxl_ctx *ctx, const char *name, char **full_name)
>              goto out;
>      } else {
>          if (errno != ENOENT)
> -            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_WARNING, "problem checking existence of"
> +            LOGE(WARN, "problem checking existence of"
>                           " logfile %s, which might have needed to be rotated",
>                           name);

Please can you reindent the trailing lines too.

Assuming they don't now fit into 80-columns perhaps you could also
rebreak the line at the clause boundary (i.e. on the comma) to aid grep.

Thanks,
Ian.

[PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in libxl_utils.c

[Kelley Nielsen]

Code cleanup - no functional changes

The convenience macros LOG and LOGE have been written to take the
place of the old macros in the LIBXL__LOG* family. Replace the
invocations of the old macros in the function libxl_read_file_contents()
with invocations of the corresponding new ones. Create a local
libxl__gc gc* for the new macros to use by invoking GC_INIT(ctx) at the
top of the function, and clean it up by invoking GC_FREE at the two
exit points.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_utils.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 5238f22..883c594 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -289,6 +289,7 @@ out:
 
 int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
                              void **data_r, int *datalen_r) {
+    GC_INIT(ctx);
     FILE *f = 0;
     uint8_t *data = 0;
     int datalen = 0;
@@ -299,23 +300,23 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
     f = fopen(filename, "r");
     if (!f) {
         if (errno == ENOENT) return ENOENT;
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to open %s", filename);
+        LOGE(ERROR, "failed to open %s", filename);
         goto xe;
     }
 
     if (fstat(fileno(f), &stab)) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to fstat %s", filename);
+        LOGE(ERROR, "failed to fstat %s", filename);
         goto xe;
     }
 
     if (!S_ISREG(stab.st_mode)) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "%s is not a plain file", filename);
+        LOGE(ERROR, "%s is not a plain file", filename);
         errno = ENOTTY;
         goto xe;
     }
 
     if (stab.st_size > INT_MAX) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "file %s is far too large", filename);
+        LOG(ERROR, "file %s is far too large", filename);
         errno = EFBIG;
         goto xe;
     }
@@ -329,9 +330,9 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
         rs = fread(data, 1, datalen, f);
         if (rs != datalen) {
             if (ferror(f))
-                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to read %s", filename);
+                LOGE(ERROR, "failed to read %s", filename);
             else if (feof(f))
-                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s changed size while we"
+                LOG(ERROR, "%s changed size while we"
                        " were reading it", filename);
             else
                 abort();
@@ -341,16 +342,18 @@ int libxl_read_file_contents(libxl_ctx *ctx, const char *filename,
 
     if (fclose(f)) {
         f = 0;
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to close %s", filename);
+        LOGE(ERROR, "failed to close %s", filename);
         goto xe;
     }
 
     if (data_r) *data_r = data;
     if (datalen_r) *datalen_r = datalen;
 
+    GC_FREE;
     return 0;
 
  xe:
+    GC_FREE;
     e = errno;
     assert(e != ENOENT);
     if (f) fclose(f);
-- 
1.8.1.2

Re: [PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in libxl_utils.c

[Ian Campbell]

On Sun, 2013-11-10 at 23:08 -0800, Kelley Nielsen wrote:
> Code cleanup - no functional changes
> 
> The convenience macros LOG and LOGE have been written to take the
> place of the old macros in the LIBXL__LOG* family. Replace the
> invocations of the old macros in the function libxl_read_file_contents()
> with invocations of the corresponding new ones. Create a local
> libxl__gc gc* for the new macros to use by invoking GC_INIT(ctx) at the
> top of the function, and clean it up by invoking GC_FREE at the two
> exit points.

In this case adding the GC_INIT was the correct approach, thanks.

>              else if (feof(f))
> -                LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "%s changed size while we"
> +                LOG(ERROR, "%s changed size while we"
>                         " were reading it", filename);

Please unwrap or reindent this one. With that changed:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

[PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in libxl_utils.c

[Kelley Nielsen]

To conform to the new coding style, replace the invocation of
LIBXL__LOG in the function libxl_pipe() in the file libxl_utils.c
with an invocation of LOG. Create a local libxl__gc gc* for LOG
to use by invoking GC_INIT(ctx) at the top of the function, and
clean it up by invoking GC_FREE at the exit. Create a variable,
ret, to consolidate exits in one place and avoid invoking GC_FREE
twice.

Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_utils.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index 883c594..8c97693 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -477,11 +477,14 @@ int libxl__remove_directory(libxl__gc *gc, const char *dirpath)
 
 int libxl_pipe(libxl_ctx *ctx, int pipes[2])
 {
+    GC_INIT(ctx);
+    int ret = 0;
     if (pipe(pipes) < 0) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Failed to create a pipe");
-        return -1;
+        LOG(ERROR, "Failed to create a pipe");
+        ret = -1;
     }
-    return 0;
+    GC_FREE;
+    return ret;
 }
 
 int libxl_uuid_to_device_vtpm(libxl_ctx *ctx, uint32_t domid,
-- 
1.8.1.2

Re: [PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in libxl_utils.c

[Ian Campbell]

On Sun, 2013-11-10 at 23:08 -0800, Kelley Nielsen wrote:
> To conform to the new coding style, replace the invocation of
> LIBXL__LOG in the function libxl_pipe() in the file libxl_utils.c
> with an invocation of LOG. Create a local libxl__gc gc* for LOG
> to use by invoking GC_INIT(ctx) at the top of the function, and
> clean it up by invoking GC_FREE at the exit. Create a variable,
> ret, to consolidate exits in one place and avoid invoking GC_FREE
> twice.
> 
> Signed-off-by: Kelley Nielsen <kelleynnn@gmail.com>
> Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

I'll apply along with the other 3 as part of the resend.

Ian.

Re: [PATCH 0/4] Compliance with new coding style

[David Vrabel]

On 11/11/13 07:08, Kelley Nielsen wrote:
> This patchset brings the file libxl_utils.c mostly into compliance
> with the new coding style involving the convenience macros LOG,
> LOGE, and GCSPRINTF.
> 
> [PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf
> [PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in
> [PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in
> [PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in

I think you should drop the "opw:" prefix from the subject.

David

Re: [PATCH 0/4] Compliance with new coding style

[Kelley Nielsen]

[-- Attachment #1.1: Type: text/plain, Size: 803 bytes --]

Yes, Ian suggested that I put it in the subject prefix. I just wanted to
give you a heads up that it was coming from someone who might not know what
they're doing :-)


On Mon, Nov 11, 2013 at 4:54 AM, David Vrabel <david.vrabel@citrix.com>wrote:

> On 11/11/13 07:08, Kelley Nielsen wrote:
> > This patchset brings the file libxl_utils.c mostly into compliance
> > with the new coding style involving the convenience macros LOG,
> > LOGE, and GCSPRINTF.
> >
> > [PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf
> > [PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in
> > [PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in
> > [PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in
>
> I think you should drop the "opw:" prefix from the subject.
>
> David
>

[-- Attachment #1.2: Type: text/html, Size: 1291 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 0/4] Compliance with new coding style

[Konrad Rzeszutek Wilk]

On Mon, Nov 11, 2013 at 05:11:09AM -0800, Kelley Nielsen wrote:
> Yes, Ian suggested that I put it in the subject prefix. I just wanted to
> give you a heads up that it was coming from someone who might not know what
> they're doing :-)

You can use git-format-patch --subject-prefix "OPW PATCH" and that would
nicely make the '[OPW PATCH] libxl'.. and when one uses git-am it strips
out the '[*]' part.

> 
> 
> On Mon, Nov 11, 2013 at 4:54 AM, David Vrabel <david.vrabel@citrix.com>wrote:
> 
> > On 11/11/13 07:08, Kelley Nielsen wrote:
> > > This patchset brings the file libxl_utils.c mostly into compliance
> > > with the new coding style involving the convenience macros LOG,
> > > LOGE, and GCSPRINTF.
> > >
> > > [PATCH 1/4] opw: libxl: use GCSPRINTF instead of libxl__sprintf
> > > [PATCH 2/4] opw: libxl: use LOGE instead of LIBXL__LOG_ERRNO in
> > > [PATCH 3/4] opw: libxl: use LOG and LOGE instead of LIBXL__LOG* in
> > > [PATCH 4/4] opw: libxl: use LOG instead of LIBXL__LOG in
> >
> > I think you should drop the "opw:" prefix from the subject.
> >
> > David
> >

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel