status subscription: extend output by joergsteffens · Pull Request #1312 · bareos/bareos

joergsteffens · 2022-11-15T11:24:00Z

While working on #1280 some improvements to the status subscription command did come up and are now handled in this separate PR.

improve the JSON output, reducing the depth of the structure and make it better understand- and parseable
add information Bareos version (also in case the calculation of units will change with other versions), current date and simple checksum.
add ACL test.

Please check

Short description and the purpose of this PR is present above this paragraph
Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

PR name is meaningful
Purpose of the PR is understood
Commit descriptions are understandable and well formatted
Check backport line
Is the PR title usable as CHANGELOG entry?
~~Separate commit for CHANGELOG.md ("update CHANGELOG.md"). The PR number is correct.~~

Source code quality

Source code changes are understandable
Variable and function names are meaningful
Code comments are correct (logically and spelling)
Required documentation changes are present and part of the PR
bareos-check-sources --since-merge does not report any problems

Tests

Decision taken that a test is required (if not, then remove this paragraph)
The choice of the type of test (unit test or systemtest) is reasonable
Testname matches exactly what is being tested
On a fail, output of the test leads quickly to the origin of the fault

arogge

Pretty good, but we can probably do better :)

arogge · 2022-11-17T14:02:56Z

core/src/cats/sql_list.cc

+    if (verbose) {
+      Mmsg(errmsg, _("Query failed: %s\n"), sql_strerror());
+      sendit->Decoration(errmsg);
+    }


This is a functional change that may not be desired.
The caller could call us with verbose set to false, resulting in errmsg still being set and using other means of displaying the message to the user.

arogge · 2022-11-17T15:08:26Z

core/src/lib/crypto_openssl.cc

+std::string compute_hash(const std::string& unhashed,
+                         const std::string& digestname)
+{
+  std::stringstream string;


please don't name variables like a type, especially don't name a variable like a different type (i.e. this is a stringstream and you named it string)

arogge · 2022-11-17T15:14:18Z

core/src/lib/crypto_openssl.cc

+      EVP_MD_CTX_destroy(context);
+    }
+  }
+  return string.str();


so on any error, you'll just get an empty string.
That doesn't feel like great error handling.
Maybe we should use an std::optional to handle errors nicer.

arogge · 2022-11-17T16:15:44Z

core/src/lib/crypto_openssl.cc

+          unsigned char hash[EVP_MAX_MD_SIZE];
+          unsigned int lengthOfHash = 0;
+          if (EVP_DigestFinal_ex(context, hash, &lengthOfHash)) {
+            string << "{" << digestname << "}";


we should probably get the digest name back from OpenSSL, so the string value is fixed, even if you pass digestname as "ShA512" or something like that.

arogge · 2022-11-17T16:18:33Z

core/src/lib/crypto_openssl.cc

+            string << "{" << digestname << "}";
+            string << std::hex << std::setw(2) << std::setfill('0');
+            for (unsigned int i = 0; i < lengthOfHash; ++i) {
+              string << (int)hash[i];


we should prefer C++-style casts like static_cast<int>() in this case over C style casts to avoid casting const away accidentally.

arogge · 2022-11-17T17:06:11Z

core/src/dird/ua_status.cc

+  PoolMem subscriptions(PM_MESSAGE);
+  PoolMem query(PM_MESSAGE);
+  OutputFormatter output_text
+      = OutputFormatter(pm_append, (void*)&subscriptions, nullptr, nullptr);


casts to void* are implicit and c-style casts are evil.

Suggested change

= OutputFormatter(pm_append, (void*)&subscriptions, nullptr, nullptr);

= OutputFormatter(pm_append, &subscriptions, nullptr, nullptr);

arogge · 2022-11-17T17:08:42Z

core/src/dird/ua_status.cc

  return true;
 }

+std::string get_subscription_status_signature_source_text(UaContext* ua,


you're only using ua->db, so maybe consider BareosDb* instead of UaContext* or maybe BareosDb&.

I'm also using ua->jcr. So either I leave it like this, or I've to add another parameter jcr. I opt for leave it like this.

arogge · 2022-11-17T17:15:31Z

core/src/cats/sql_list.cc

                      description, verbose);
 }

+bool BareosDb::ListOneRowSqlQuery(JobControlRecord* jcr,


instead of adding a whole new function, we could consider adding another defaulted parameter to ListSqlQuery() that will make it use ObjectStart()/ObjectEnd() if SqlNumRows() == 1.

Optimal would be a custom enum class with reasonable symbolic names so that you're passing something like CollapseMode::Collapse and default it to CollapseMode::NoCollapse instead of just adding a bool where you have to look up what it does.

Good point.

arogge · 2022-11-17T17:16:46Z

core/src/dird/ua_status.cc

+    std::string signature_source
+        = get_subscription_status_signature_source_text(ua, now);
+    std::string hash = compute_hash(signature_source.c_str());
+    ua->send->ObjectKeyValue("signature", "Signature: ", hash.c_str(), "%s\n");


I really wouldn't call it a signature, it is just a hash or checksum, so we should probably name it as such.

I named it Checksum now.

arogge · 2022-11-17T17:18:09Z

core/src/dird/ua_status.cc

+    ua->send->ObjectKeyValue("binary-info",
+                             "Binary info: ", kBareosVersionStrings.BinaryInfo,
+                             "%s\n");
+    ua->send->ObjectKeyValue("date", "Date: ", now, "%s\n");


It is called date, but contains the current time.
Maybe something like "report-time" and "Report time:" is more reasonable.

joergsteffens · 2022-12-01T16:18:44Z

I addressed all mentioned suggestions. Without new feedback, the only I maybe can address is replacing compute_hash by the already existing function. Anyhow, I'll not have the time for this before next week. But maybe this is not even required, as this function also works on all our supported platforms.

arogge

I made a few observations, but no objections. Looks good to me now!

arogge · 2022-12-07T11:35:44Z

core/src/dird/ua_status.cc

+    std::optional<std::string> checksum = compute_hash(checksum_source.c_str());
+    if (checksum) {
+      ua->send->ObjectKeyValue("checksum",
+                               "Checksum: ", checksum.value().c_str(), "%s\n");
+    }


i would probably have used auto instead of std::optional<std::string> here.

compute_hash() takes a const std::string& as first parameter. You're passing the underlying const char* of your existing std::string to it, which will lead to an implicit conversion to std::string() which implies another copy. Just passing the std::string would be better

there are two ways to get the value out of a std::optional: calling value() or operator*/operator->. With value() you get a check that there is a value in your optional (and an exception if there is none), while with the operators behaviour is undefined if there is no value. As you checked that there is a value, you can safely use the operators, simplifying checksum.value().c_str() to just checksum->c_str().

Instead of the command ACL "configure", unrestriced access to Clients, Jobs and Filesets is required, as these are the information that the command really uses.

This can result is much better read- and parseable results.

In case the calculation of subscription will change in the future, adding version information will help to determine how the subscription counting is done at that time.

joergsteffens added the jenkins_repotest_nodebug label Nov 15, 2022

joergsteffens requested a review from arogge November 15, 2022 11:24

bruno-at-bareos assigned arogge Nov 17, 2022

arogge requested changes Nov 17, 2022

View reviewed changes

joergsteffens assigned joergsteffens and unassigned arogge Nov 28, 2022

pstorz added the Release critical 22 label Dec 1, 2022

pstorz requested a review from alaaeddineelamri December 1, 2022 11:23

joergsteffens requested a review from arogge December 1, 2022 16:13

arogge approved these changes Dec 7, 2022

View reviewed changes

frb121 and others added 3 commits December 8, 2022 10:12

status subscription: without command ACL configure

4cef162

Instead of the command ACL "configure", unrestriced access to Clients, Jobs and Filesets is required, as these are the information that the command really uses.

core: add option for DB queries as single row

909697d

This can result is much better read- and parseable results.

status subscription: improve JSON output

47f54b5

joergsteffens force-pushed the dev/joergs/master/subscription-core branch from 6b752d9 to be7080d Compare December 8, 2022 15:02

joergsteffens added 3 commits December 8, 2022 16:43

status subscription: write version information

55ff759

In case the calculation of subscription will change in the future, adding version information will help to determine how the subscription counting is done at that time.

status subscription: add checksum

e42d96a

systemtests: add 'status subscription' ACL test

592c5db

arogge force-pushed the dev/joergs/master/subscription-core branch 2 times, most recently from 2614462 to 592c5db Compare December 8, 2022 16:59

Update CHANGELOG.md

47cc12e

arogge merged commit 787ba1e into bareos:master Dec 8, 2022

frb121 mentioned this pull request Dec 14, 2022

webui: introduce subscription backup unit report #1280

Merged

17 tasks

joergsteffens deleted the dev/joergs/master/subscription-core branch October 11, 2023 08:08

	= OutputFormatter(pm_append, (void*)&subscriptions, nullptr, nullptr);
	= OutputFormatter(pm_append, &subscriptions, nullptr, nullptr);

Conversation

joergsteffens commented Nov 15, 2022 • edited by arogge Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

General

Source code quality

Tests

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joergsteffens commented Dec 1, 2022

Uh oh!

arogge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joergsteffens commented Nov 15, 2022 •

edited by arogge

Loading