8:48 a.m.
Select non-comment labels using an augeas path to return the correct nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
---
src/inspect_fs_unix.c | 50 +++++++++++++++++++++---------------------------
1 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/src/inspect_fs_unix.c b/src/inspect_fs_unix.c
index 0fa3e83..34d218f 100644
--- a/src/inspect_fs_unix.c
+++ b/src/inspect_fs_unix.c
@@ -110,7 +110,6 @@ compile_regexps (void)
COMPILE (re_scientific_linux_no_minor,
"Scientific Linux.*release (\\d+)", 0);
COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
- COMPILE (re_aug_seq, "/\\d+$", 0);
COMPILE (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0);
COMPILE (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0);
COMPILE (re_mdN, "^(/dev/md\\d+)$", 0);
@@ -132,7 +131,6 @@ free_regexps (void)
pcre_free (re_scientific_linux);
pcre_free (re_scientific_linux_no_minor);
pcre_free (re_major_minor);
- pcre_free (re_aug_seq);
pcre_free (re_xdev);
pcre_free (re_cciss);
pcre_free (re_mdN);
@@ -725,47 +723,43 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
Hash_table *md_map;
if (!map_md_devices (g, &md_map)) return -1;
- char **lines = guestfs_aug_ls (g, "/files/etc/fstab");
- if (lines == NULL) goto error;
+ char *augpath = NULL;
+ char **entries = guestfs_aug_match (g, "/files/etc/fstab/*"
+ "[label() != '#comment']");
+ if (entries == NULL) goto error;
- if (lines[0] == NULL) {
+ if (entries[0] == NULL) {
error (g, _("could not parse /etc/fstab or empty file"));
goto error;
}
- size_t i;
- char augpath[256];
- for (i = 0; lines[i] != NULL; ++i) {
- /* Ignore comments. Only care about sequence lines which
- * match m{/\d+$}.
- */
- if (match (g, lines[i], re_aug_seq)) {
- snprintf (augpath, sizeof augpath, "%s/spec", lines[i]);
- char *spec = guestfs_aug_get (g, augpath);
- if (spec == NULL) goto error;
-
- snprintf (augpath, sizeof augpath, "%s/file", lines[i]);
- char *mp = guestfs_aug_get (g, augpath);
- if (mp == NULL) {
- free (spec);
- goto error;
- }
+ for (char **entry = entries; *entry != NULL; entry++) {
+ if (asprintf (&augpath, "%s/spec", *entry) == -1) g->abort_cb();
+ char *spec = guestfs_aug_get (g, augpath);
+ if (spec == NULL) goto error;
- int r = add_fstab_entry (g, fs, spec, mp, md_map);
+ if (asprintf (&augpath, "%s/file", *entry) == -1) g->abort_cb();
+ char *mp = guestfs_aug_get (g, augpath);
+ if (mp == NULL) {
free (spec);
- free (mp);
-
- if (r == -1) goto error;
+ goto error;
}
+
+ int r = add_fstab_entry (g, fs, spec, mp, md_map);
+ free (spec);
+ free (mp);
+
+ if (r == -1) goto error;
}
hash_free (md_map);
- guestfs___free_string_list (lines);
+ guestfs___free_string_list (entries);
return 0;
error:
hash_free (md_map);
- if (lines) guestfs___free_string_list (lines);
+ if (entries) guestfs___free_string_list (entries);
+ free(augpath);
return -1;
}
--
1.7.7.3
1:13 p.m.
On Thu, Nov 24, 2011 at 02:48:46PM +0000, Matthew Booth wrote:
Select non-comment labels using an augeas path to return the correct
nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
In general this is an obvious improvement.
+ for (char **entry = entries; *entry != NULL; entry++) {
+ if (asprintf (&augpath, "%s/spec", *entry) == -1) g->abort_cb();
We have a safe_asprintf function defined already, and it would be
better to use that.
Is augpath supposed to be freed on the non-error path?
spec and mp should be freed on the error path, but don't seem to be at
the moment.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into Xen guests.
http://et.redhat.com/~rjones/virt-p2v
4:02 a.m.
On 11/24/2011 07:13 PM, Richard W.M. Jones wrote:
On Thu, Nov 24, 2011 at 02:48:46PM +0000, Matthew Booth wrote:
> Select non-comment labels using an augeas path to return the correct nodes in
> the first instance, rather than applying a regular expression to all results.
> Iterate over returned matches using a char** iterator.
> Use asprintf() to ensure the path string buffer is the correct size.
In general this is an obvious improvement.
> + for (char **entry = entries; *entry != NULL; entry++) {
> + if (asprintf (&augpath, "%s/spec", *entry) == -1)
g->abort_cb();
We have a safe_asprintf function defined already, and it would be
better to use that.
Is augpath supposed to be freed on the non-error path?
spec and mp should be freed on the error path, but don't seem to be at
the moment.
I was mis-remembering the definition of asprintf(). I thought it was
doing a realloc() on the buffer you passed in, but reading it again I
don't see that. I'll respin it.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
4:25 a.m.
Select non-comment labels using an augeas path to return the correct nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
---
src/inspect_fs_unix.c | 50 +++++++++++++++++++++---------------------------
1 files changed, 22 insertions(+), 28 deletions(-)
diff --git a/src/inspect_fs_unix.c b/src/inspect_fs_unix.c
index 0fa3e83..34d218f 100644
--- a/src/inspect_fs_unix.c
+++ b/src/inspect_fs_unix.c
@@ -110,7 +110,6 @@ compile_regexps (void)
COMPILE (re_scientific_linux_no_minor,
"Scientific Linux.*release (\\d+)", 0);
COMPILE (re_major_minor, "(\\d+)\\.(\\d+)", 0);
- COMPILE (re_aug_seq, "/\\d+$", 0);
COMPILE (re_xdev, "^/dev/(h|s|v|xv)d([a-z]+)(\\d*)$", 0);
COMPILE (re_cciss, "^/dev/(cciss/c\\d+d\\d+)(?:p(\\d+))?$", 0);
COMPILE (re_mdN, "^(/dev/md\\d+)$", 0);
@@ -132,7 +131,6 @@ free_regexps (void)
pcre_free (re_scientific_linux);
pcre_free (re_scientific_linux_no_minor);
pcre_free (re_major_minor);
- pcre_free (re_aug_seq);
pcre_free (re_xdev);
pcre_free (re_cciss);
pcre_free (re_mdN);
@@ -725,47 +723,43 @@ check_fstab (guestfs_h *g, struct inspect_fs *fs)
Hash_table *md_map;
if (!map_md_devices (g, &md_map)) return -1;
- char **lines = guestfs_aug_ls (g, "/files/etc/fstab");
- if (lines == NULL) goto error;
+ char *augpath = NULL;
+ char **entries = guestfs_aug_match (g, "/files/etc/fstab/*"
+ "[label() != '#comment']");
+ if (entries == NULL) goto error;
- if (lines[0] == NULL) {
+ if (entries[0] == NULL) {
error (g, _("could not parse /etc/fstab or empty file"));
goto error;
}
- size_t i;
- char augpath[256];
- for (i = 0; lines[i] != NULL; ++i) {
- /* Ignore comments. Only care about sequence lines which
- * match m{/\d+$}.
- */
- if (match (g, lines[i], re_aug_seq)) {
- snprintf (augpath, sizeof augpath, "%s/spec", lines[i]);
- char *spec = guestfs_aug_get (g, augpath);
- if (spec == NULL) goto error;
-
- snprintf (augpath, sizeof augpath, "%s/file", lines[i]);
- char *mp = guestfs_aug_get (g, augpath);
- if (mp == NULL) {
- free (spec);
- goto error;
- }
+ for (char **entry = entries; *entry != NULL; entry++) {
+ if (asprintf (&augpath, "%s/spec", *entry) == -1) g->abort_cb();
+ char *spec = guestfs_aug_get (g, augpath);
+ if (spec == NULL) goto error;
- int r = add_fstab_entry (g, fs, spec, mp, md_map);
+ if (asprintf (&augpath, "%s/file", *entry) == -1) g->abort_cb();
+ char *mp = guestfs_aug_get (g, augpath);
+ if (mp == NULL) {
free (spec);
- free (mp);
-
- if (r == -1) goto error;
+ goto error;
}
+
+ int r = add_fstab_entry (g, fs, spec, mp, md_map);
+ free (spec);
+ free (mp);
+
+ if (r == -1) goto error;
}
hash_free (md_map);
- guestfs___free_string_list (lines);
+ guestfs___free_string_list (entries);
return 0;
error:
hash_free (md_map);
- if (lines) guestfs___free_string_list (lines);
+ if (entries) guestfs___free_string_list (entries);
+ free(augpath);
return -1;
}
--
1.7.7.3
6:54 a.m.
On 11/25/2011 10:25 AM, Matthew Booth wrote:
Select non-comment labels using an augeas path to return the correct
nodes in
the first instance, rather than applying a regular expression to all results.
Iterate over returned matches using a char** iterator.
Use asprintf() to ensure the path string buffer is the correct size.
---
src/inspect_fs_unix.c | 50 +++++++++++++++++++++---------------------------
1 files changed, 22 insertions(+), 28 deletions(-)
I just noticed this doesn't have any changes in it :) It's genuinely
coming up in a sec.
I went back to using a buffer on the stack, but added error checking.
Also note that spec and md were already being freed as necessary on the
error paths. That code didn't change.
Matt
--
Matthew Booth, RHCA, RHCSS
Red Hat Engineering, Virtualisation Team
GPG ID: D33C3490
GPG FPR: 3733 612D 2D05 5458 8A8A 1600 3441 EA19 D33C 3490
4670
days inactive
4671
days old
4 comments
2 participants
participants (2)
-
Matthew Booth -
Richard W.M. Jones