Re: [Libguestfs] [PATCH 1/2] ext2: tweak the error returned message of resize2fs-M(BZ755729)

2012-01-13 Thread Richard W.M. Jones 
On Fri, Jan 13, 2012 at 02:27:49PM +0800, Wanlong Gao wrote:
 Tweak the error message e2fsck -f and e2fsck -fy to
 e2fsck-f and e2fsck-fy.
 
 Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
 ---
  daemon/ext2.c |   20 ++--
  1 files changed, 18 insertions(+), 2 deletions(-)
 
 diff --git a/daemon/ext2.c b/daemon/ext2.c
 index 79fd354..9fe938e 100644
 --- a/daemon/ext2.c
 +++ b/daemon/ext2.c
 @@ -277,9 +277,25 @@ do_resize2fs_M (const char *device)
if (e2prog (prog) == -1)
  return -1;
  
 -  r = command (NULL, err, prog, -M , device, NULL);
 +  r = command (NULL, err, prog, -M, device, NULL);
if (r == -1) {
 -reply_with_error (%s, err);
 +int i = 0;
 +int len = strlen (err);

Use 'size_t' (instead of 'int') for the return value of strlen.

What is 'i' used for?

 +char *err_wrap = malloc (len + 1);
 +if (!err_wrap)
 +  goto err_out;

You need to call 'reply_with_error' or 'reply_with_perror' exactly
once on each error path.  So here you'd need to call
  reply_with_perror (malloc);
before the 'goto'.

 +char *p = strstr (err, e2fsck -f);
 +if (p) {
 +  strncpy (err_wrap, err, p - err);

Isn't it better to use memcpy here?  AFAIK using strncpy is almost
always wrong.  (Jim?)

 +  err_wrap[p - err] = '\0';
 +  sprintf(err_wrap + (p - err), %s%s, e2fsck-f, p + 9);
 +  reply_with_error (%s, err_wrap);
 +} else {
 +  reply_with_error (%s, err);
 +}
 +
 +free (err_wrap);
 +err_out:
  free (err);
  return -1;
}

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 80 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH 1/2] ext2: tweak the error returned message of resize2fs-M(BZ755729)

2012-01-13 Thread Wanlong Gao 
On 01/13/2012 06:08 PM, Richard W.M. Jones wrote:

 On Fri, Jan 13, 2012 at 02:27:49PM +0800, Wanlong Gao wrote:
 Tweak the error message e2fsck -f and e2fsck -fy to
 e2fsck-f and e2fsck-fy.

 Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
 ---
  daemon/ext2.c |   20 ++--
  1 files changed, 18 insertions(+), 2 deletions(-)

 diff --git a/daemon/ext2.c b/daemon/ext2.c
 index 79fd354..9fe938e 100644
 --- a/daemon/ext2.c
 +++ b/daemon/ext2.c
 @@ -277,9 +277,25 @@ do_resize2fs_M (const char *device)
if (e2prog (prog) == -1)
  return -1;
  
 -  r = command (NULL, err, prog, -M , device, NULL);
 +  r = command (NULL, err, prog, -M, device, NULL);
if (r == -1) {
 -reply_with_error (%s, err);
 +int i = 0;
 +int len = strlen (err);
 
 Use 'size_t' (instead of 'int') for the return value of strlen.


yeah.

 
 What is 'i' used for?


mistake, forgot to remove it.

 
 +char *err_wrap = malloc (len + 1);
 +if (!err_wrap)
 +  goto err_out;
 
 You need to call 'reply_with_error' or 'reply_with_perror' exactly
 once on each error path.  So here you'd need to call
   reply_with_perror (malloc);
 before the 'goto'.


yes, I see.

 
 +char *p = strstr (err, e2fsck -f);
 +if (p) {
 +  strncpy (err_wrap, err, p - err);
 
 Isn't it better to use memcpy here?  AFAIK using strncpy is almost
 always wrong.  (Jim?)


yes, you are right, I'll change to memcpy.


Thanks a lot
-Wanlong Gao

 
 +  err_wrap[p - err] = '\0';
 +  sprintf(err_wrap + (p - err), %s%s, e2fsck-f, p + 9);
 +  reply_with_error (%s, err_wrap);
 +} else {
 +  reply_with_error (%s, err);
 +}
 +
 +free (err_wrap);
 +err_out:
  free (err);
  return -1;
}
 
 Rich.
 


___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs

Re: [Libguestfs] [PATCH 1/2] ext2: tweak the error returned message of resize2fs-M(BZ755729)

2012-01-13 Thread Jim Meyering 
Richard W.M. Jones wrote:
 On Fri, Jan 13, 2012 at 02:27:49PM +0800, Wanlong Gao wrote:
 Tweak the error message e2fsck -f and e2fsck -fy to
 e2fsck-f and e2fsck-fy.

 Signed-off-by: Wanlong Gao gaowanl...@cn.fujitsu.com
 ---
  daemon/ext2.c |   20 ++--
  1 files changed, 18 insertions(+), 2 deletions(-)

 diff --git a/daemon/ext2.c b/daemon/ext2.c
 index 79fd354..9fe938e 100644
 --- a/daemon/ext2.c
 +++ b/daemon/ext2.c
 @@ -277,9 +277,25 @@ do_resize2fs_M (const char *device)
if (e2prog (prog) == -1)
  return -1;

 -  r = command (NULL, err, prog, -M , device, NULL);
 +  r = command (NULL, err, prog, -M, device, NULL);
if (r == -1) {
 -reply_with_error (%s, err);
 +int i = 0;
 +int len = strlen (err);

 Use 'size_t' (instead of 'int') for the return value of strlen.

 What is 'i' used for?

 +char *err_wrap = malloc (len + 1);
 +if (!err_wrap)
 +  goto err_out;

 You need to call 'reply_with_error' or 'reply_with_perror' exactly
 once on each error path.  So here you'd need to call
   reply_with_perror (malloc);
 before the 'goto'.

 +char *p = strstr (err, e2fsck -f);
 +if (p) {
 +  strncpy (err_wrap, err, p - err);

 Isn't it better to use memcpy here?  AFAIK using strncpy is almost
 always wrong.  (Jim?)

I would.  The fact that strncpy does not always NUL-terminate
makes it hard to use correctly.  Better to avoid it in general.

 +  err_wrap[p - err] = '\0';
 +  sprintf(err_wrap + (p - err), %s%s, e2fsck-f, p + 9);

Also, as a general rule, it is easier to code and maintain
uses of asprintf than separate malloc+sprintf.
You can depend on asprintf, since it is provided by gnulib, if needed.

And, technically sprintf can fail (with glibc, it may even invoke malloc!),
so you should handle failure.

___
Libguestfs mailing list
Libguestfs@redhat.com
https://www.redhat.com/mailman/listinfo/libguestfs